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

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

Orlando added a comment.

Thanks!



================
Comment at: llvm/lib/Transforms/Utils/CloneFunction.cpp:719-720
+      RemapInstruction(NewDVI, VMap,
+                       ModuleLevelChanges ? RF_None : RF_NoModuleLevelChanges,
+                       TypeMapper, Materializer);
+  }
----------------
jmorse wrote:
> 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.
Yeah, that should all be okay. 


================
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())
----------------
jmorse wrote:
> Would a clearer test be `!DAI->getDebugLoc().getInlinedAt()`? (I think they're basically the same thing, doesn't really matter)
That looks right (well, inverted) - done.


================
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",
----------------
jmorse wrote:
> Check for existence of "inlinedAt" too, even if we don't really care about the node?
We actually don't want `inlinedAt` here - the `dbg.assign` that is inserted is for the caller's variable (and the variable and dbg intrinsic scopes need to match).  I've updated the CHECK line to explicitly check against seeing an `inlinedAt` field. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133318