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

[David Stenberg via Phabricator via llvm-commits]

dstenb marked 2 inline comments as done.
dstenb added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp:5356
+
+  if (Reg != MI.getOperand(0).getReg())
+    return None;
----------------
vsk wrote:
> Probably worth adding a comment here similar to the ones elsewhere, about cases where the add defines a super/sub register of the described reg not being supported yet.
Yes!


================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:7707
+    Expr = DIExpression::get(MI.getMF()->getFunction().getContext(), {});
+    if (X86MCRegisterClasses[X86::GR64RegClassID].contains(Reg))
+      Expr = DIExpression::appendExt(Expr, 32, 64, true);
----------------
vsk wrote:
> IIUC, at this point, `Reg` must be a sub-register of operand 0 or equal to it. Can operand 0 of a MOVSX64rr32 ever not be 64-bit, and if not, is this check necessary?
The MachineVerifier complains if operand 0 is not a 64-bit register. That check is still necessary as it's there to detect if the described register is 32/64 bits. However, I don't understand why I did it that complicated instead of just using a simple `(Reg == MI.getOperand(0).getReg)` check.

I'll switch to `(Reg == MI.getOperand(0).getReg())`. I'll also add an assert that the described register otherwise is the 32 bit sub-register, so that we don't silently produce incorrect call site values for 8- and 16-bit sub-registers. AFAICT we currently can never land in a situation where we need to describe a 8- or 16-bit sub-register, but perhaps later on we will.


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

https://reviews.llvm.org/D70431