[PATCH] D70431: [DebugInfo] Make describeLoadedValue() reg aware

[Vedant Kumar via Phabricator via llvm-commits]

vsk added inline comments.


================
Comment at: llvm/lib/CodeGen/TargetInstrInfo.cpp:1148
+    // zero-extending copy.
+    if (TRI->isSuperRegisterEq(DestReg, Reg) && MI.definesRegister(Reg, TRI))
+      return ParamLoadedValue(*Source, Expr);
----------------
I think this should either be `TRI->isSuperRegister(...)`, or the previous `Reg == DestReg` condition should be deleted. My preference would be to keep the cases separate, as I find that clearer.


================
Comment at: llvm/lib/CodeGen/TargetInstrInfo.cpp:1148
+    // zero-extending copy.
+    if (TRI->isSuperRegisterEq(DestReg, Reg) && MI.definesRegister(Reg, TRI))
+      return ParamLoadedValue(*Source, Expr);
----------------
vsk wrote:
> I think this should either be `TRI->isSuperRegister(...)`, or the previous `Reg == DestReg` condition should be deleted. My preference would be to keep the cases separate, as I find that clearer.
Why is it safe to assume the copy would be zero-extending here?


================
Comment at: llvm/lib/CodeGen/TargetInstrInfo.cpp:1155
+    if (unsigned SubRegIdx = TRI->getSubRegIndex(DestReg, Reg))
+      if (unsigned SrcSubReg = TRI->getSubReg(Source->getReg(), SubRegIdx))
+        return ParamLoadedValue(MachineOperand::CreateReg(SrcSubReg, false),
----------------
Is it possible for the source & destination subregister indices to be different, in a copy? E.g.: if there were an instruction which copied the high 32 bits of %r8 into the low 32 bits of %r9. If so, maybe subregister copies would be safer to handle within specializations of `describeLoadedValue`.

If not, or we just don't know of any such cases, there should be a clear note about this assumption added to the `isCopyInstr` documentation.


================
Comment at: llvm/lib/CodeGen/TargetInstrInfo.cpp:1174
+    // zero-extending add immediate.
+    if (TRI->isSuperRegisterEq(DestReg, Reg) && MI.definesRegister(Reg, TRI))
+      return ParamLoadedValue(*Source, Expr);
----------------
Ditto, not sure why 'isSuperRegister' isn't enough here.


================
Comment at: llvm/lib/CodeGen/TargetInstrInfo.cpp:1174
+    // zero-extending add immediate.
+    if (TRI->isSuperRegisterEq(DestReg, Reg) && MI.definesRegister(Reg, TRI))
+      return ParamLoadedValue(*Source, Expr);
----------------
vsk wrote:
> Ditto, not sure why 'isSuperRegister' isn't enough here.
Ditto, why is it safe to assume this is a zero-extending add? E.g. I think 'sxta' on ARM works differently.


================
Comment at: llvm/lib/CodeGen/TargetInstrInfo.cpp:1202
+
+    // TODO: In what way do we need to take Reg into consideration here?
+
----------------
dstenb wrote:
> A bit related to the above comment, when working on this patch I encountered a case where an incorrect call site value is emitted when describing a 32-bit value on x86-64: https://bugs.llvm.org/show_bug.cgi?id=43343#c8.
Thanks for catching this!


================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:7565
+         dwarf::DW_OP_LLVM_convert, ToBits, dwarf::DW_ATE_signed});
+    return DIExpression::get(MF->getFunction().getContext(), Ops);
+  };
----------------
Might be cleaner to stash the `LLVMContext &` in a helper local var.


================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:7566
+    return DIExpression::get(MF->getFunction().getContext(), Ops);
+  };
+
----------------
Can this just be a helper method in DIExpression? It's possible that replaceAllDbgUsesWith could use this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70431