[PATCH] D60716: [DwarfDebug] Dump call site debug info into DWARF

[Djordje Todorovic via Phabricator via llvm-commits]

djtodoro marked 12 inline comments as done.
djtodoro added a comment.

@vsk Thanks for the comments!



================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:580
+  for (auto ArgReg : CallFwdRegsInfo->second)
+    ArgsRegsForProcess.insert(ArgReg.Reg);
+
----------------
vsk wrote:
> djtodoro wrote:
> > vsk wrote:
> > > Can we assert that this insertion into ArgsRegsForProcess occurs? I'm assuming that it's a bug if more than one argument is forward in the same register.
> > > 
> > > Super nit-pick, but, I don't think having 'ForProcess' in the name is very clear. Something like 'ForwardedRegWorklist' would be more in keeping with the naming in the rest of the compiler.
> > This only adds all the regs into the working list by iterating through the `CallSiteInfo`.
> > 
> > >Super nit-pick, but, I don't think having 'ForProcess' in the name is very clear. Something like 'ForwardedRegWorklist' would be more in keeping with the naming in the rest of the compiler.
> > 
> > Thanks!!
> I see. I had in mind:
> 
> ```
> bool InsertedReg = ForwardedRegWorklist.insert(ArgReg.Reg).second;
> assert(InsertedReg && "Single register used to forward two arguments?");
> ```
> 
> If this is already checked elsewhere, there's no need for a duplicate assert.
I see.. That makes sense.


================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:609
+          if (TRI->regsOverlap(FwdReg, MO.getReg()))
+            Defs.push_back(FwdReg);
+      }
----------------
vsk wrote:
> After FwdReg from MO is pushed into Defs, shouldn't this loop should break? IIUC no other registers should be equal-to/alias FwdReg.
Yes. Thanks!


================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:631
+    // forwarding registers or we finished the interpretation of all parameters.
+    if (I->isCall())
+      return;
----------------
vsk wrote:
> Should the basic block scan stop at branches in addition to calls to reduce compile time (MI::isCall() || MI::isBranch())?
Actually, that is not necessary, since this will never encounter a branch.


================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:632
+    if (I->isCall())
+      return;
+
----------------
vsk wrote:
> If the scan reaches a call or a branch, shouldn't it break instead of returning? That way, the ShouldTryEmitEntryVals case can be finished. Perhaps there should be a test?
If it encounters a call that means we can not consider the 'DW_OP_entry_value' as valid expression with in a call site parameter value, since the call may clobber that, and we stop the analysis as is.

There is a test for that part within the //test/DebugInfo/MIR/X86/dbgcall-site-interpretation.mir//.


================
Comment at: lib/CodeGen/AsmPrinter/DwarfExpression.cpp:479
+        emitUnsigned(Op->getArg(0));
+        emitSigned(Op->getArg(1));
+      } else {
----------------
vsk wrote:
> I don't understand the lowering of DW_OP_regx within addMachineRegExpression. Why should the lowering be different within/without parameter values? It seems like the DIExpression itself should be formed with DW_OP_bregx, if/when needed.
Yes, thanks! That is better way for sure.


================
Comment at: lib/CodeGen/TargetInstrInfo.cpp:1146
+      return None;
+    unsigned CastedOffset = static_cast<unsigned>(Offset);
+    if (Offset > 0)
----------------
vsk wrote:
> I do not think that casting the offset to unsigned is safe due to loss of precision. Also, note that the expression generated for the Offset=0 case is inefficient. Is there some way to reuse the code in DIExpression::appendOffset for this?
I agree, we can use the `DIExpression::appendOffset`.


================
Comment at: lib/Target/X86/X86InstrInfo.cpp:7452
+      Elements.push_back(dwarf::DW_OP_plus);
+    }
+
----------------
vsk wrote:
> We should be able to factor out the logic in DIExpression::{append,prepend}Offset to simplify this.
We can use that.


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

https://reviews.llvm.org/D60716