[PATCH] D133318: [Assignment Tracking][21/*] Account for assignment tracking in inliner

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

Looks good, with the caveat that I think you need to explicitly check for DIAssignID nodes in the output of tests to make FileCheck notice if something is merged / distinct (see inline comments).



================
Comment at: llvm/lib/Transforms/Utils/CloneFunction.cpp:385-389
+    // nodes for which we defer processing until we update the CFG. Also skip
+    // debug intrinsics because we should preserve use-before-defs, which are
+    // well(-ish) defined and explicitly well defined for assignment
+    // tracking. To preserve use-before-defs we must remap debug intrinsic
+    // operands after all values have been mapped.
----------------
I'd suggest not mentioning assignment tracking at all here, that won't be a consideration of the "average" reader. IMO it should be alright to curtly mention that debug intrinsics are handled later, and shift any commentary on use-before-def to wherever the debug intrinsics are maintained. (This is purely for the purpose of not burdening the reader with too much information).


================
Comment at: llvm/lib/Transforms/Utils/CloneFunction.cpp:719-720
+      RemapInstruction(NewDVI, VMap,
+                       ModuleLevelChanges ? RF_None : RF_NoModuleLevelChanges,
+                       TypeMapper, Materializer);
+  }
----------------
Just so that I understand, vmap is a mapping from the old function to the duplicated instructions in the new -- so we're only remapping those debug intrinsics that survived the inlining?

(This seems fine to me).


================
Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:1564
+/// Find Alloca and linked DbgAssignIntrinsic for locals escaped by \p CB.
+static DenseMap<const AllocaInst *, SmallSet<at::VarRecord, 2>>
+collectEscapedLocals(const DataLayout &DL, const CallBase &CB) {
----------------
I feel the reader will be happier if there's a typedef for this return type (visually easier to parse too).


================
Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:1630
+    DIAssignID *OldID = cast<DIAssignID>(Old);
+    if (DIAssignID *NewID = Map.lookup(OldID))
+      return NewID;
----------------
It feels like a slightly abnormal usage that `Map.lookup` is going to default-allocate the NewID as nullptr, so `lookup` also initializes. AFAIUI there's nothing wrong with this though.


================
Comment at: llvm/test/DebugInfo/Generic/assignment-tracking/inline/id.ll:4-6
+;; Check that all DIAssignID metadata that are inlined are replaced with new
+;; versions. Otherwise two inlined instances of an assignment will be considered
+;; to be the same assignment.
----------------
The inlined (_Z3funv) function is in the output as well -- IMO there's value in testing that it's distinct from the other IDs.

Also, unless you check at the end for the existence of two DIAssignID nodes with dedicated numbers, I think ID_0 and ID_1 can end up having the same value, and this isn't an error as far as FileCheck is concerned.


================
Comment at: llvm/test/DebugInfo/Generic/assignment-tracking/inline/inline-stores.ll:176
+; CHECK:       store i32 5, i32* %f5_param.addr, align 4,{{.*}}!DIAssignID ![[ID_5:[0-9]+]]
+; CHECK-NEXT:  call void @llvm.dbg.assign(metadata i32 5, metadata ![[f5_param:[0-9]+]], metadata !DIExpression(), metadata ![[ID_5]], metadata i32* %f5_param.addr, metadata !DIExpression())
+define dso_local i32 @_Z2f5i(i32 %f5_param) #3 !dbg !86 {
----------------
As there are two variables !88 !89 in this function, shouldn't we check for both variables in the CHECK lines?


================
Comment at: llvm/test/DebugInfo/Generic/assignment-tracking/inline/inline-stores.ll:215
+
+; CHECK-DAG: ![[f1_local]] = !DILocalVariable(name: "f1_local",
+; CHECK-DAG: ![[f2_local]] = !DILocalVariable(name: "f2_local",
----------------
As with earlier, you'll need to check for DIAssignID nodes to ensure that none are merged / duplicated accidentally.


================
Comment at: llvm/test/DebugInfo/Generic/assignment-tracking/inline/use-before-def.ll:20-23
+;; NOTE: Although this reproducer is contrived, this has been observed in real
+;; builds with assignment tracking (dbg.assign intrinsics) - in fact, it caused
+;; verifier failures in some cases using assignment tracking (good). This is
+;; the simplest test I could write.
----------------
SGTM -- having a well defined meaning for this kind of scenario is exactly the outcome we should see from treating assignments as assignments in the compiler.


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

https://reviews.llvm.org/D133318