[PATCH] D88891: [DebugInfo][InstrRef][1/4] Support transformations that widen or narrow defined values

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/MachineFunction.h:463
+  /// later.
+  std::map<DebugInstrOperandPair, DebugSubstitution> DebugValueSubstitutions;
 
----------------
djtodoro wrote:
> Do we need the `std::map` sortness here?
Hmmm -- not during the optimisation passes, so this can become a vector that gets sorted when lookups occur.

(I have another patch (not uploaded yet) that might need the sortness, but that can be evaluated in its own right).


================
Comment at: llvm/include/llvm/CodeGen/MachineFunction.h:486
+  void makeDebugValueSubstitution(DebugInstrOperandPair, DebugInstrOperandPair,
+                                  unsigned SubReg = 0);
 
----------------
StephenTozer wrote:
> Also not so much a nit or request as a question; should this default value be taken to mean that it is okay for the substitution's subreg to be larger than the source operand, with such a case being treated as a no-op rather than a sext/zext? I'd assume so, since I don't believe that we would ever want to produce a sext/zext using this substitution mechanism - that should only happen as a result of salvaging.
That would be my interpretation, yeah -- while a reproducer doesn't immediately spring to mind, I can't think of a part of post-isel debug-info that minds if $al is expanded to $rax when it's the operand of a DBG_VALUE. I'm confident I've seen it in the past, but can't put my finger on it right now.

In theory, as future work, we could eliminate that situation if we perfectly instrumented the entire backend with substitution subregister info, but I don't think we would gain anything from that.


================
Comment at: llvm/lib/CodeGen/MIRParser/MIRParser.cpp:421
     MF.makeDebugValueSubstitution(std::make_pair(Sub.SrcInst, Sub.SrcOp),
-                                  std::make_pair(Sub.DstInst, Sub.DstOp));
   }
----------------
StephenTozer wrote:
> Complete nit, but is there a reason for the `std::make_pair` -> `{}` substitution?
IIRC it's the preferred form, and I was taking the opportunity to fix it up as this line changes anyway.

Not sure why I didn't do the other make_pair, I'll edit that in.


================
Comment at: llvm/lib/Target/X86/X86FixupBWInsts.cpp:381
+  if (unsigned OldInstrNum = MI->peekDebugInstrNum()) {
+    unsigned Subreg = TRI->getSubRegIndex(MIB->getOperand(0).getReg(),
+                                          MI->getOperand(0).getReg());
----------------
StephenTozer wrote:
> StephenTozer wrote:
> > Also one more question about the intended behaviour (that does not block this patch at all), it's potentially expected that we may have to recurse through several substitutions? As is I don't think there's any issue with that, since I believe there is a small lower bound on the number of substitutions we could ever need to make for a single reference, and it seems easier than checking for existing substitutions everywhere that we might create one.
> s/lower/upper
Indeed, multiple substitutions might be made -- the number will be proportionate to the amount of optimisation that has happened to a value, so it shouldn't inflate massively. It's the realisation of the overall theme: don't maintain a perfectly accurate mapping of variable locations during compilation, instead document what happens and piece it together later.


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

https://reviews.llvm.org/D88891