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

[David Stenberg via Phabricator via llvm-commits]

dstenb added inline comments.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:649
+    SmallVector<unsigned, 4> ExplicitDefs;
+    SmallVector<unsigned, 4> ImplicitDefs;
+    getForwardingRegsDefinedByMI(*I, ExplicitDefs, ImplicitDefs);
----------------
vsk wrote:
> Consider renaming these vectors with 's/Defs/ForwardedDefs/g'. This would clarify that these defs are both a) attached to I and b) forwarded.
I renamed them to the slightly shorter `FwdRegDefs` to avoid some awkward indentation due to >80 cols.


================
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);
----------------
vsk wrote:
> auto Reg : concat<unsigned>(ExplicitDefs, ImplicitDefs) ?
Neat!


================
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:
> 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 ?
Oh, yes. Thanks for catching that!


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

https://reviews.llvm.org/D67225