[PATCH] D80345: [DwarfExpression] Support entry values for indirect parameters

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added inline comments.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:1288
   DwarfExpr.addFragmentOffset(DIExpr);
-  if (Location.isIndirect())
+  if (Location.isIndirect() && !DIExpr->isEntryValue())
     DwarfExpr.setMemoryLocationKind();
----------------
Can you add a comment? it's not necessarily obvious why an indirect entry value is not a memory location. Is it because an entry value is its own kind?


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:790
+    // FIXME: This produces unusable descriptions when the register contains
+    // a pointer to a temporary copy of a struct passed by value.
     DIExpression *EntryExpr = DIExpression::get(
----------------
What does "unusable" mean? Incorrect? Invalid?


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:2404
     MachineLocation Location = Value.getLoc();
-    if (Location.isIndirect())
+    if (Location.isIndirect() && !DIExpr->isEntryValue())
       DwarfExpr.setMemoryLocationKind();
----------------
same here


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:2412
     }
 
     const TargetRegisterInfo &TRI = *AP.MF->getSubtarget().getRegisterInfo();
----------------
factoring out this snippet into a template and reusing it in DwarfCompileUnit.cpp is probably not going to make this more readable, is it?


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfExpression.h:146
 
   /// The flags of location description being produced.
+  enum {
----------------
This comment is weird :-)

`Additional flags that may be combined with any location description kind.`?


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfExpression.h:149
+    EntryValue = 1 << 0,
+    IndirectEntryValue = 1 << 1,
+    CallSiteParamValue = 1 << 2
----------------
Would it make more sense call this `Indirect` (w/o EntryValue) and treat it as a separate bit, so you can still test for EntryValue independently?


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfExpression.h:308
   /// Lock this down to become an entry value location.
-  void setEntryValueFlag() { LocationFlags |= EntryValue; }
+  void setEntryValueFlagFromLoc(MachineLocation Loc) {
+    LocationFlags |= Loc.isIndirect() ? IndirectEntryValue : EntryValue;
----------------
Is the dependency on MachineLocation really necessary, or could this just take a bool / enum IsIndirect?


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfExpression.h:309
+  void setEntryValueFlagFromLoc(MachineLocation Loc) {
+    LocationFlags |= Loc.isIndirect() ? IndirectEntryValue : EntryValue;
+  }
----------------
see comment in header...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80345