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

Thu Feb 7 05:22:55 PST 2019

bjope marked 2 inline comments as done.
bjope added inline comments.

================
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
----------------
bjope wrote:
> 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).
I've run a bunch of tests (including random programs generated by csmith) and with the changes in EmitFuncArgumentDbgValue I haven't seen any situation when EmitFuncArgumentDbgValue would return true here any longer (and in the past that seems to have been incorrect). So I really think we shold remove the call to EmitFuncArgumentDbgValue here. But I rather do it in a separate patch after this one.

================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:4997
+    // emit using ArgDbgValue. This might catch some situations when the
+    // dgg.value refers to an argument that isn't used in the entry block, so
+    // any CopyToReg node would be optimized out and the only way to express
----------------
jmorse wrote:
> Typo dgg -> dbg
Thanks, I'll fix that!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57702