[PATCH] D57702: [SelectionDAGBuilder] Add restrictions to EmitFuncArgumentDbgValue

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

LGTM although I'll wait for someone else to approve, plus it'd be good to hear Adrians view on the fragment scenario.



================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:5025-5032
+    if (VariableIsFunctionInputArg) {
+      unsigned ArgNo = Arg->getArgNo();
+      if (ArgNo >= FuncInfo.DescribedArgs.size())
+        FuncInfo.DescribedArgs.resize(ArgNo + 1);
+      else if (!IsInPrologue && FuncInfo.DescribedArgs.test(ArgNo))
+        return false;
+      FuncInfo.DescribedArgs.set(ArgNo);
----------------
bjope wrote:
> jmorse wrote:
> > Will this work with Arguments that are referred to by multiple fragments? Perhaps it doesn't need to, however the two-i32s-packed-in-i64-argument in PR40188 sprung to mind as that produces two DBG_VALUEs based on one Argument.
> At one point in time I tried using `ArgNo = Variable->getArg()`, looking at the C-level argument numbers, but then I had a test case that failed due to receiving that single struct variable in multiple registers (multiple fragments).
> Now I use the argument numbers from the function on IR level. So at least that test case passed, since we get one fragment per input register. I guess it will be the same if the value is divided in multiple stack slots.
> 
> So I actually had problem with fragments in my first attempts to fix this, but I have no (real, non handwritten IR) test cases failing with the current solution.
> Maybe there can be scenarios when we have multiple fragments mapping to one argument on IR level? (probably easy to do when doing a handwritten test case, but does it happen in reality)
> 
> So we might need to "complicate" this code even more in the future, or even find a totally different solution (the special case handling for function arguments seems a little bit messy IMHO). Maybe the frontend shouldn't use dbg.declare/dbg.value to describe input arguments, maybe we need dbg.argument or some other way to model this (if we really care about debug info for unused arguments, and if we really care about debug info inside the prologue)? 
> 
> For now, as far as I've seen (mainly when looking at C-programs), this patch seems to improve things. Avoiding faulty placement of DBG_VALUE nodes due to incorrectly treating them as describing function arguments. There is a small risk that it is too defensive in some situations. But instead of getting weird values (like with the faulty hoisting), being defensive in this case means that we would get <optimized out> in the debugger.
> [...] a test case that failed due to receiving that single struct variable in multiple registers (multiple fragments)

Ah, my use of "fragments" was too vague, it was intended to mean:

> Maybe there can be scenarios when we have multiple fragments mapping to one argument on IR level?

Which I tried to produce an example for, but I hit some extra bugs along the way: [0]. In the example code there [0] the struct argument comes in as one i64, and SROA splits the dbg.declare into two dbg.values that refer to different variable fragments in their DIExpressions, but both ultimately refer to the same input Argument. If it weren't for [0] I believe (95%) they'd be salvaged back to two dbg.values operating on the input Argument.

> For now, as far as I've seen (mainly when looking at C-programs), this patch seems to improve things.

I agree, the argument-handling-weirdness has bugged me for a while, and this patch reduces the amount false debug information we produce. If multiple fragments of one Argument can be an issue we'll definitely hit it when solving [0].

[0] https://bugs.llvm.org/show_bug.cgi?id=40645


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57702