[PATCH] D57584: [DebugInfo][DAG] Reduce SelectionDAGs reordering of variables referring to Arguments

Jeremy Morse via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Feb 4 10:58:18 PST 2019


jmorse added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:5357
+    // need to let them dangle until they get an SDNode.
+    bool isParamOfFunc = isa<Argument>(V) && Variable->isParameter() &&
+                         Variable->getScope() == FuncInfo.Fn->getSubprogram();
----------------
aprantl wrote:
> jmorse wrote:
> > aprantl wrote:
> > > bjope wrote:
> > > > Capitalize local variable name.
> > > Wouldn't this condition incorrectly also identify an inlined argument of a recursive function call as being an argument of the outermost non-inlined function? I think you may also need to check the inlinedAt of the dbg.value is null.
> > Ah yes, that would break. I've added a test that calls DISubprogram->describes here, lifted from r317702, would that be sufficient?
> I don't think that works, because the debug info of an inlined recursive call will also describe the same function. My understanding is that we want this condition to apply to all dbg.values that describe parameters of the function but not inlined valrables? If that's the case, the missing condition is `DbgValueInst->getDebugLocation()->getInlinedAt() == nullptr`.
> 
> But perhaps I'm misunderstanding what we want here.
> 
> ```
> void @f(myObj*) !dbg !4 {
>   // Non-inlined this.
>   call @llvm.dbg.value(metadata %0, metadata !1, metadata !DIExpression()), !dbg !2
>   // Inlined instance of this of recursive function call.
>   call @llvm.dbg.value(metadata %0, metadata !1, metadata !DIExpression()), !dbg !3
> ...
> 
> !1 = !DILocalVariable(name "this", ...)
> !2 = !DILocation(scope: !4, line: 42)
> !3 = !DILocation(scope: !4, line: 42, inlinedAt: !5)
> ```
Aha -- I'd misunderstood the original suggestion as meaning "inlined params of the recursive functions should be treated like the outermost function", whoops. Thanks for the clarity.


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

https://reviews.llvm.org/D57584





More information about the llvm-commits mailing list