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

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:4983
+        Variable->getScope() != FuncInfo.Fn->getSubprogram() ||
+        FuncInfo.MBB != &FuncInfo.MF->front())
+      return false;
----------------
aprantl wrote:
> This condition looks very much like the condition in https://reviews.llvm.org/D57584. Could it be factored out into a helper function with a reasonable name and documented behavior?
Yes, I'm kind of monitoring the progress of D57584 to see where we end up with these checks (I'm now using the getInlinedAt kind of check here as well). Either we can wait and refactor after these patches has landed, or we can perhaps create a common solution (in one of the patches) based on which patch that lands first.
I got a feeling that somehow they might overlap (both patches are reducing amount of hoisted DBG_VALUE nodes to the entry).




================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:1159
     if (Val.getNode()) {
+      // FIXME: I doubt that it is correct to resolve a dangling DbgValue as a
+      // FuncArgumentDbgValue (it would be hoisted to the function entry, and if
----------------
I'll investigate this a little bit more. I have a reproducer where this results would result in incorrect hosting (but that is on trunk). With this patch EmitFuncArgumentDbgValue returns false for that reproducer.
But adding an assert in the else clause (after applying this patch), and then running some test suites might give us some new reproducers (or indicate that the EmitFuncArgumentDbgValue()" call is redundant here).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57702