[PATCH] D67225: [DebugInfo][X86] Describe call site values for zero-valued imms

[Vedant Kumar via Phabricator via llvm-commits]

vsk added a comment.

Thanks for working on this!

Having ParamLoadedValue contain a MachineOperand object makes sense to me. This would stave off an explosion of ad hoc OneOperand, TwoOperand, etc forever-objects. And you've demonstrated that the overhead is negligible.

Looking forward, I also think it makes sense to change the describeLoadedValue API s.t. it's specific about which loaded value is interesting. That can wait until there's a motivating test case.



================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:601
+  // registers, add those defines. We can currently only describe forwarded
+  // register that are explicitly defined, but keep track of implicit defines
+  // also to remove those registers from the work list.
----------------
registers


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:649
+    SmallVector<unsigned, 4> ExplicitDefs;
+    SmallVector<unsigned, 4> ImplicitDefs;
+    getForwardingRegsDefinedByMI(*I, ExplicitDefs, ImplicitDefs);
----------------
Consider renaming these vectors with 's/Defs/ForwardedDefs/g'. This would clarify that these defs are both a) attached to I and b) forwarded.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:656
     // all of them from the working list.
-    for (auto Reg : Defs)
+    for (auto Reg : ExplicitDefs)
       ForwardedRegWorklist.erase(Reg);
----------------
auto Reg : concat<unsigned>(ExplicitDefs, ImplicitDefs) ?


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:665
+    // least partially) defined by the instruction's single explicit define.
+    if (I->getNumExplicitDefs() != 1 && ExplicitDefs.empty())
       continue;
----------------
I think this is a reasonable limitation to enforce until we have a clearer picture of how to handle instructions with multiple forwarded defines.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:665
+    // least partially) defined by the instruction's single explicit define.
+    if (I->getNumExplicitDefs() != 1 && ExplicitDefs.empty())
       continue;
----------------
vsk wrote:
> I think this is a reasonable limitation to enforce until we have a clearer picture of how to handle instructions with multiple forwarded defines.
Should this be: I->getNumExplicitDefs() != 1 || ExplicitDefs.empty ?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D67225