[PATCH] D88896: [DebugInfo][InstrRef][3/4] Produce DBG_INSTR_REFs for all* variable locations

[Orlando Cazalet-Hyams via Phabricator via llvm-commits]

Orlando added a comment.

I've taken a stab at reviewing this. I especially appreciate and enjoy the detailed comments and patch description. I've left some nits and questions inline but otherwise it looks good IMO.



================
Comment at: llvm/include/llvm/CodeGen/MachineFunction.h:510
+  ///          value, or None if nothing could be recovered.
+  Optional<DebugInstrOperandPair> salvageCopySSA(MachineInstr &MI);
+
----------------
Does this need to return an `Optional`? As far as I can tell `salvageCopySSA` never returns `None`, and the only callsite doesn't check whether the returned result has a value or not.


================
Comment at: llvm/lib/CodeGen/MachineFunction.cpp:1039-1040
+      Offset = TRI.getSubRegIdxOffset(SubReg);
+    if (OldRegBits >= NewRegBits || Offset)
+      SubReg = 0;
+
----------------
I'm struggling to follow this part. When you say "only remember truncations or offsets" am I right in thinking that setting `SubReg = 0` causes some kind of mismatch that is picked up and handled by LiveDebugValues/InstrRefBasedImpl in D88894?


================
Comment at: llvm/lib/CodeGen/MachineFunction.cpp:1133-1140
+  auto Builder = BuildMI(TargetBB, TargetBB.getFirstNonPHI(), DebugLoc(),
+                         TII.get(TargetOpcode::DBG_PHI));
+  Builder.addReg(State.first);
+  unsigned NewNum = getNewDebugInstrNum();
+  Builder.addImm(NewNum);
+  MachineInstr *NewInst = Builder;
+  NewInst->getOperand(0).setIsDebug(true);
----------------
Is it worth putting the create-a-dbg-phi code into a function that is shared between these patches? No strong opinion on this.


================
Comment at: llvm/lib/CodeGen/MachineFunction.cpp:1139
+  MachineInstr *NewInst = Builder;
+  NewInst->getOperand(0).setIsDebug(true);
+  return ApplySubregisters({NewNum, 0u});
----------------
nit: I think you can do `Builder.addReg(State.first, RegState::Debug)` on line 1135 instead.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp:776
+  // SelectionDAG when more information is available.
+  auto EmitHalfDoneInstrRef = [&](unsigned VReg) -> MachineInstr * {
+    auto MIB = BuildMI(*MF, DL, RefII);
----------------
SGTM. I make a comment later about moving the SelectionDAGISel half-done-dbg-instr-fixup code into its own function. If you do that you could change `and patch it up later in SelectionDAG` to `and patch it up later in <function-name>`, which might make it easier to quickly find the relevant code in the future.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/InstrEmitter.h:121-128
+  /// Emit a DBG_VALUE $noreg, indicating a variable has no location.
+  MachineInstr *EmitDbgNoLocation(SDDbgValue *SD);
+
+  /// Emit a DBG_VALUE %stack, referring to a frame index.
+  MachineInstr *EmitDbgFrameIndex(SDDbgValue *SD);
+
+  /// Emit a DBG_VALUE of some constant value.
----------------
It might be useful to split the factoring-out of this DBG_VALUE building code from `EmitDbgValue` into a separate NFC patch IMO, but it's a probably not too important.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp:646-700
+  auto MakeDbgValue = [&](MachineInstr &MI) {
+    const MCInstrDesc &RefII = TII->get(TargetOpcode::DBG_VALUE);
+    MI.setDesc(RefII);
+    MI.getOperand(1).ChangeToRegister(0, false);
+    MI.getOperand(0).setIsDebug();
+  };
+
----------------
I think it would be good to move this fixup-half-done-instr-refs code into its own function. It would reduce clutter in `runOnMachineFunction` and you could then also add a comment to the `EmitHalfDoneInstrRef ` lambda mentioning it so that it's easy to find from there. wdyt?


================
Comment at: llvm/test/DebugInfo/X86/instr-ref-selectiondag.ll:83
+
+;; Don't test the location of these instr-refs, only that the three non-argument
+;; dbg.values become DBG_INSTR_REFs. We previously checked that these numbers
----------------
Sorry if this is mentioned elsewhere - why are you explicitly not testing the location (position?) of the isntr-refs here?


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

https://reviews.llvm.org/D88896