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

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

Orlando added 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.
----------------
jmorse wrote:
> 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).
Is this OK?


================
Comment at: llvm/lib/Transforms/Utils/CloneFunction.cpp:719-720
+      RemapInstruction(NewDVI, VMap,
+                       ModuleLevelChanges ? RF_None : RF_NoModuleLevelChanges,
+                       TypeMapper, Materializer);
+  }
----------------
jmorse wrote:
> 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).
> Just so that I understand, vmap is a mapping from the old function to the duplicated instructions in the new

I believe so.

>  so we're only remapping those debug intrinsics that survived the inlining?

What do you mean by survived the inlining?


================
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) {
----------------
jmorse wrote:
> I feel the reader will be happier if there's a typedef for this return type (visually easier to parse too).
Added `StorageToVarsMap` in D132225  but hadn't gotten round to updating this yet. Done.


================
Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:1630
+    DIAssignID *OldID = cast<DIAssignID>(Old);
+    if (DIAssignID *NewID = Map.lookup(OldID))
+      return NewID;
----------------
jmorse wrote:
> 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.
The `DenseMap::lookup` docu-comment says `Return the entry for the specified key, or a default constructed value if no such entry exists` so I figured it was ok.


================
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.
----------------
jmorse wrote:
> 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.
Good point, done.


================
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 {
----------------
jmorse wrote:
> As there are two variables !88 !89 in this function, shouldn't we check for both variables in the CHECK lines?
This test is just checking that the inlined stores are tracked correctly (e.g. the dbg.assign for f5_param that already exists in f5 prior to inlining isn't checked for here either). I think there's value in keeping this test targeted - wdyt?


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

https://reviews.llvm.org/D133318