[PATCH] D136320: [Assignment Tracking Analysis][1/*] Add analysis pass core

Thu Nov 24 04:14:35 PST 2022

jmorse added a comment.

Looks good with some nits fixed, a spurious return removed, and the question on line 825 addressed.

Of course, landing this would require some tests; I believe you'll get stern looks if this lands with nothing covering it. Thus you might want to fold in the tests from patch 5 as appropriate, when they're reviewed.

================
Comment at: llvm/include/llvm/CodeGen/AssignmentTrackingAnalysis.h:65
+  const VarLocInfo *single_locs_end() const {
+    return VarLocRecords.begin() + SingleVarLocEnd;
+  }
----------------
jmorse wrote:
> I recommend std::advance to do exactly the same thing, but making it very clear to the reader that you're messing with iterators, rather than having to consider that there's pointer arithmetic happening. Here and elsewhere.
(done below, but here appreciated too, not a blocker)

================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:655
+  // earlier location definitions, and in many cases it should be a reasonable
+  // assumption. However, this will occasionally lead slight inaccuracies. The
+  // value of a hoisted untagged store will be visible "early", for example.
----------------

================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:897
+
+void AssignmentTrackingLowering::resetInsertionPoint(Instruction *After) {
+  assert(!After->isTerminator() && "Can't insert after a terminator");
----------------
Reference rather than pointer given it's unconditionally dereferenced?

================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:899
+  assert(!After->isTerminator() && "Can't insert after a terminator");
+  return;
+  auto R = InsertBeforeMap.find(After->getNextNode());
----------------
Leftover return from prototyping?

================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:164
+
+void FunctionVarLocs::init(FunctionVarLocsBuilder *Builder) {
+  // Add the single-location variables first.
----------------
jmorse wrote:
> Reference argument instead of pointer? It's unconditionally dereferenced.
While we're at it, `const auto &`

================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:360
+    /// that we can't deduce the LocKind of a variable by just looking at the
+    /// two maps.
+    LocMap LiveLoc;
----------------
Orlando wrote:
> jmorse wrote:
> > This is another one of those places where I feel the word "dominance frontier" can be inserted to a) make ourselves feel clever, and b) actually disambiguate what's going on. i.e., "NoneOrPhi indicates whether there is a single dominating definition which can be found in StackHomeValue or DebugValue". Or something like that? (Might not be right).
> That code comment is indeed confusing at best. I've had a go at rewording it, let me know what you think. Happy to make further changes.
Much more understandable now, cheers!

================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:383-384
+  FunctionVarLocsBuilder *FnVarLocs;
+  DenseMap<const BasicBlock *, BlockInfo> LiveIn;
+  DenseMap<const BasicBlock *, BlockInfo> LiveOut;
+
----------------
Orlando wrote:
> jmorse wrote:
> > These translate to maps of maps again, which risks an expensive reallocation. There isn't necessarily anything to do about this if that's not triggered by the workloads that actaully exist.
> I'm not sure that these ones can be avoided. These are at least initialized to a size equal to the number of basic blocks, down in `AssignmentTrackingLowering::run`.
Hrrrmmm. I suppose over in LiveDebugValues we have exactly this, but with a SmallVector of DenseMaps where the block number is the index. That's pretty much identical to `LiveIn` / `LiveOut` here. Given this is a dataflow algorithm, random access to blocks is unavoidable, and within those random access to variables, which requires two levels of map.

That being said, I eventually found it faster to deal with a single variable at a time and trade one locality with another. But again, we should only re-visit this if it turns out that there's a performance loss.

================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:413-414
+  ///
+  /// For the map-type functions, all unmapped keys are assumed to be
+  /// associated with a top value (⊤).
+  ///@{
----------------
Orlando wrote:
> jmorse wrote:
> > This wants explaining why -- it's because the top value represents "don't know", right?
> I've elaborated a bit. Is this ok or does it need more?
SGTM

================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:425-426
+  /// Process the instructions in \p BB updating \p LiveSet along the way. \p
+  /// LiveSet must be initialized with the current live-in locations before
+  /// calling this.
+  void process(BasicBlock &BB, BlockInfo *LiveSet);
----------------
Orlando wrote:
> jmorse wrote:
> > If it's initialized, pass by reference instead?
> I chose to use a pointer to hint at call sites that it is modified. If that is not compelling (or the hint is not doing its job) I can change it - just thought I'd offer up my rationale first in case it made a difference.
(I think this was applied to `process` before the comments floated) This is moderately a style thing, so up to you in the end.

================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:474
+
+  // Update the LocKind for all fragments contained within Var.
+  for (VariableID Frag : VarContains[Var])
----------------
Orlando wrote:
> jmorse wrote:
> > Var itself is a fragment, right? Small risk that the reader thinks it's a DILocalVariable? (Our terminology doesn't help).
> Yes (if by "is a fragment" you mean is an ID that identifies a `{DILocalVariable, FragmentInfo, InlinedAt}` tuple aka a `DebugVariable`, which indeed includes fragment information). I don't think I've deviated massively from common naming practices but I agree that our terminology isn't necessarily helpful here. `DebugVariable` should probably be called something like `VariableInstanceFragment` (but that is outside the scope of this patch of course). Do you have any suggestions for `Var` / `VariableID`?
Nah, just making sure I understood correctly.

================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:647
+    // Alloca or non-store-like inst.
+    return (Optional<at::AssignmentInfo>)None;
+  }();
----------------
Orlando wrote:
> jmorse wrote:
> > Is this cast necessary? Might be better to put a trailing type on the lambda to make it un-necessary? Any particular need for a lambda instead of something else?
> Ah, `AssignmentInfo` has no default ctor so we can't do this:
> ```
> AssignmentInfo Info;
> if ...
>     Info = ....
> else if ...
>      Info = ...
> ```
> 
> Let me know if you'd prefer that though as I can just add one.
No need, a potentially un-necessary lambda is better than opening the scope of "bugs we can introduce by allowing default-constructed data structures".

================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:824
+          dbgs() << "Val, Stack matches Debug program but address is undef\n";);
+      setLocKind(LiveSet, Var, LocKind::Val);
+    } else {
----------------
Orlando wrote:
> jmorse wrote:
> > It looks slightly out of place that the LocKind is recorded as a Val, but then emitDbgValue is used with LocKind::Mem -- I suppose the problem is that I don't know what's meant by elision of the original store. What are the effects of this scenario {LocKind==Val,emitDbgValue(Mem...)}?
> > It looks slightly out of place that the LocKind is recorded as a Val, but then emitDbgValue is used with LocKind::Mem
> 
> It does look slightly out of place. This could be a bug - I'll get back to you on this one.
(This space intentionally left blank -- to signal to myself that there'll be a response coming back from you at some point).

================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:975
+    return Assignment::makeNoneOrPhi();
+  if (A.Status == Assignment::NoneOrPhi)
+    return Assignment::makeNoneOrPhi();
----------------
Orlando wrote:
> jmorse wrote:
> > Coming immediately after the leading comment, shouldn't this also check if `A.Status == NoneOrPhi`?
> That is checked right here above your review comment. I used separate `if`s for readability, but happy to combine them if this was counterproductive?
Nah, I'm just blind apparently

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

https://reviews.llvm.org/D136320