[PATCH] D67556: [ARM][AArch64][DebugInfo] Improve call site instruction interpretation

Mon Oct 14 06:24:40 PDT 2019

dstenb added a comment.

Thanks for adding the source-level reproducers!

LGTM except for the some minor inline comments. I think it would be good to let the others have their say about this also.

================
Comment at: lib/CodeGen/TargetInstrInfo.cpp:1137-1140
+    // TODO: For now, there is no use of describing the value loaded into the
+    //       register that is also the source registers (e.g. $r0 = add $r0, x).
+    if (DestRegOp->getReg() == SrcRegOp->getReg())
+      return None;
----------------
Would it be safe to return such values without this check due to the `getNumElements() == 0` check in `collectCallSiteParameters()`? If so, perhaps we should just consider this a limitation in `collectCallSiteParameters()`, and let the hook describe the values even though they currently can't be used by the caller.

================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:5698-5700
+                             const MachineOperand *&Destination,
+                             const MachineOperand *&Source,
+                             int &Offset) const {
----------------
nit: Indentation. There are some minor indentation things inside the switch case also (e.g. space before parenthesis in `assert (`).

================
Comment at: lib/Target/ARM/ARMBaseInstrInfo.cpp:5335
+  // TODO: Third operand can be global address (usually some string). Since
+  //       strings can be relocated we cannot calculate theirs offsets for
+  //       now.
----------------
dstenb wrote:
> nit: //theirs -> their//
That typo is still here.

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

https://reviews.llvm.org/D67556