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

[Djordje Todorovic via Phabricator via llvm-commits]

djtodoro added a comment.

I like the way of the implementation. Thanks!



================
Comment at: llvm/include/llvm/CodeGen/TargetInstrInfo.h:74
 };
 
+struct RegImmediatePair {
----------------
Please add a comment here explaining the usage of the struct.

Maybe better `RegImmPair` ?


================
Comment at: llvm/include/llvm/CodeGen/TargetInstrInfo.h:960
   /// If the specific machine instruction is an instruction that adds an
-  /// immediate value to its source operand and stores it in destination,
-  /// return destination and source registers as machine operands along with
-  /// \c Offset which has been added.
-  virtual Optional<DestSourcePair> isAddImmediate(const MachineInstr &MI,
-                                                  int64_t &Offset) const {
+  /// immediate value and a physical register and stores the result in
+  /// the given physical register \c Reg, return a pair of the source
----------------
`and a physical register and stores, and stores the`


================
Comment at: llvm/lib/CodeGen/TargetInstrInfo.cpp:1144
+    // be handled by the target's hook implementation.
+    assert(!TRI->isSuperOrSubRegisterEq(Reg, DestReg) &&
+           "TargetInstrInfo::describeLoadedValue can't describe super- or "
----------------
This looks good to me!


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:5789
+static Optional<ParamLoadedValue>
+describeOrrLoadedValue(const MachineInstr &MI, Register DescribedReg,
+                       const TargetInstrInfo *TII,
----------------
`describeOrrLoadedValue` ==> `describeORRLoadedValue` ?


================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:7557
+static Optional<ParamLoadedValue>
+describeMovrrLoadedValue(const MachineInstr &MI, Register DescribedReg,
+                         const TargetRegisterInfo *TRI) {
----------------
`describeMovrrLoadedValue` ==> `describeMOVrrLoadedValue` ?


================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:7568
+
+  // If the described register if a sub-register of the destination register,
+  // then pick out the source register's corresponding sub-register.
----------------
`is a sub-register`


================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:7586
+  assert(MI.getOpcode() == X86::MOV32rr && "Unexpected super-register case");
+  return ParamLoadedValue(MachineOperand::CreateReg(SrcReg, false), Expr);
+}
----------------
So, this covers all the cases of the s//uper-registers// except the `X86::MOV32rr` case?


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

https://reviews.llvm.org/D70431