[PATCH] D132225: [Assignment Tracking][6/*] Add trackAssignments function

[Orlando Cazalet-Hyams via Phabricator via llvm-commits]

Orlando added inline comments.


================
Comment at: llvm/include/llvm/IR/DIBuilder.h:910
+    DbgAssignIntrinsic *insertDbgAssign(Instruction *LinkedInstr, Value *Val,
+                                        DILocalVariable &SrcVar,
+                                        DIExpression *ValExpr, Value *Addr,
----------------
jmorse wrote:
> Why a reference for DILocalVariable -- convention (as I understand it) elsewhere is to pass all metadata as pointers.
Fair, not sure why I did that. Fixed.


================
Comment at: llvm/include/llvm/IR/DebugInfo.h:235
+/// Track assignments to \p Vars between \p Start and \p End.
+/// FIXME: Backing storage shouldn't be limited to allocas only. Some local
+/// variables have their storage allocated by the calling function (addresses
----------------
jmorse wrote:
> I'd say TODO rather than FIXME,
Done and I've included this on the TODO list in the docs in D132220.


================
Comment at: llvm/include/llvm/IR/DebugInfo.h:239
+void trackAssignments(
+    Function::iterator Start, Function::iterator End,
+    const DenseMap<const AllocaInst *, SmallSet<VarRecord, 2>> &Vars,
----------------
jmorse wrote:
> Possibly silly question, but does this ever get called with iterators other than begin() and end()? I don't get the feeling that a range of blocks is ever useful (is that how inlining inserts them, as a range?).
Yes it is because of the inliner (see D133318).


================
Comment at: llvm/include/llvm/IR/Value.h:750-754
+  /// Strip off pointer casts and GEPs.
+  ///
+  /// Returns the original pointer value.  If this is called on a non-pointer
+  /// value, it returns 'this'.
+  const Value *stripOffsets() const;
----------------
jmorse wrote:
> I want to say that there must be something in LLVM that already does this, but I can only see things that deal with inbound geps, or constants. Could I suggest a name like "stripAllOffsets", to give the reader a strong hint that this method drops / ignores a _lot_ of information -- that'll discourage people from using it in contexts where they need to care about some of the offsets. (Renaming can be in a follow-up patch or something, no need to re-juggle the patch stack because of a name nitpick).
It looks like this is actually just code lying around from earlier on in the prototype so I've removed it, sorry for the noise.


================
Comment at: llvm/lib/IR/DebugInfo.cpp:1693-1694
+
+/// Collect constant properies (base, size, offset) of \p StoreDest.
+/// Return None if any properties are not constants.
+static Optional<AssignmentInfo> getAssignmentInfoImpl(const DataLayout &DL,
----------------
jmorse wrote:
> Just to ask the obvious question: are you able to use `Value::stripAndAccumulateConstantOffsets` instead of this?
I vaguely remember asking myself this same question. I can't remember what conclusions I came to and yet this function remains here. Nevertheless, I really can't see a reason why we can't use `stripAndAccumulateConstantOffsets` instead and in fact building CTMark's tramp3d-v4 with -emit-llvm results in no difference in output with and without this change (at least while `AllowNonInbounds` is true). Odd, but I'm happy to chalk it up to being product of coming out of the prototype stage.


================
Comment at: llvm/lib/IR/DebugInfo.cpp:1735
+  // Assume 8 bit bytes.
+  auto *ConstOffset = dyn_cast<ConstantInt>(I->getOperand(2));
+  if (!ConstOffset)
----------------
jmorse wrote:
> Is this field actually an offset as the variable name suggests? My (shallow) reading of MemIntrinsic suggests that it's the length (aka size?) of the load/store being performed, which is not an offset.
You're right - that's just a badly named variable. Fixed, nice catch, thanks.


================
Comment at: llvm/lib/IR/DebugInfo.cpp:1769
+  }
+  DIExpression *AddrExpr = DIExpression::get(StoreLikeInst.getContext(), None);
+  return DIB.insertDbgAssign(&StoreLikeInst, &Val, *VarRec.Var, Expr, &Dest,
----------------
jmorse wrote:
> Feels like you can just re-use the identical call to DIExpression::get from above.
`Expr` gets re-assigned in the if block below its declaration, so they're not always identical.


================
Comment at: llvm/lib/IR/DebugInfo.cpp:1781-1783
+  // Early-exit if there are no interesting variables.
+  if (Vars.empty())
+    return;
----------------
jmorse wrote:
> Any need for "if Start == End return"?
It's not needed for correctness (that's the loop stop condition and nothing happens after the loop) and I can't imagine that it's a case common enough to warrant the early-out. Happy to do it though if you think it's worth it.


================
Comment at: llvm/lib/IR/DebugInfo.cpp:1910-1918
+    for (DbgDeclareInst *DDI : P.second) {
+      // FIXME: Are there any that wouldn't get converted now? Better to be
+      // safe and still check for now.
+      if (std::find_if(Markers.begin(), Markers.end(),
+                       [DDI](DbgAssignIntrinsic *DAI) {
+                         return DAI->getVariable() == DDI->getVariable();
+                       }) != Markers.end())
----------------
jmorse wrote:
> Not completely clear what the find is doing, comment appreciated. Promote to an assertion perhaps, or EXPENSIVE_CHECKS?
I've improved the comment and upgraded the `if` to an assert. I think it should be okay as just an assert and not wrapped in EXPENSIVE_CHECKS because typically `Markers` will only contain one element. I've also improved the check from DILocalVariable == DILocalVariable to include scope & fragment info by using `DebugVariable`. I'm using the new `DebugVariable` ctor added in D133286, so I've added that patch as a parent.


================
Comment at: llvm/lib/IR/DebugInfo.cpp:1925-1926
+  runOnFunction(F);
+  // Q: Can we return a less conservative set than just CFGAnalyses? Can we
+  // return PreservedAnalyses::all()?
+  PreservedAnalyses PA;
----------------
jmorse wrote:
> IMHO: we should be able to specify that all analyses are preserved. If they're not, that's a debug-info-changes-codegen bug.
Sorry I should have mentioned that I was copying what `RedundantDbgInstElimination` (`redundant-dbg-inst-elim`) in llvm/lib/Transforms/Scalar/DCE.cpp does. Should I still change it to `PreservedAnalyses::all()`?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132225/new/

https://reviews.llvm.org/D132225