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

[Jeremy Morse via Phabricator via llvm-commits]

jmorse accepted this revision.
jmorse added a comment.
This revision is now accepted and ready to land.

LGTM with a couple of suggestions. The bigger comment is me asking "have you considered what happens when the inliner deletes random elements of assignment tracking due to pruning" -- I don't think there's anything that would be broken, just checking that we have a common understanding.



================
Comment at: llvm/lib/Transforms/Utils/CloneFunction.cpp:719-720
+      RemapInstruction(NewDVI, VMap,
+                       ModuleLevelChanges ? RF_None : RF_NoModuleLevelChanges,
+                       TypeMapper, Materializer);
+  }
----------------
Orlando wrote:
> 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?
> What do you mean by survived the inlining?

Blocks that are determined to be unreachable during inlining are pruned away and, presumably, their debug intrinsics don't get cloned into the new function, hence it's possible for the lookup of DVI in VMap to be null. I think I wrote this comment to see if that was a code path you'd considered, in case dbg.assigns need to behave differently when things are pruned.

It's possible for blocks containing dbg.assigns to be pruned, but that should act the same as any old dead code elimination. It's probably not possible for blocks containing allocas with DIAssignIDs to be pruned but corresponding dbg.assigns not be pruned -- the dbg.assigns will be in blocks dominated by the alloca. Presumably nothing goes wrong if there are dbg.assigns with linked stores that don't get cloned, or vice versa.


================
Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:1602-1604
+      // Skip variables from inlined functions - they are not local variables.
+      if (DAI->getVariable()->getScope()->getSubprogram() !=
+          CB.getFunction()->getSubprogram())
----------------
Would a clearer test be `!DAI->getDebugLoc().getInlinedAt()`? (I think they're basically the same thing, doesn't really matter)


================
Comment at: llvm/test/DebugInfo/Generic/assignment-tracking/inline/inline-stores.ll:222
+
+; CHECK-DAG: ![[f1_dbg]] = !DILocation({{.*}}scope: ![[f1_scope:[0-9]+]]{{.*}})
+; CHECK-DAG: ![[f1_scope]] = distinct !DISubprogram(name: "f1",
----------------
Check for existence of "inlinedAt" too, even if we don't really care about the node?


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

https://reviews.llvm.org/D133318