[PATCH] D114452: [DebugInfo][InstrRef][X86] Instrument expanded DYN_ALLOCA_ instructions with instruction numbers

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

Orlando added a comment.

Makes sense to me from a debug info perspective, and tests LGTM. I've added a couple of nits inline. It might be worth someone else having a quick look as I'm not familiar with the .cpps touched here.



================
Comment at: llvm/lib/Target/X86/X86DynAllocaExpander.cpp:217
+  if (unsigned Num = MI->peekDebugInstrNum())
+    // Operand 2 of DYN_ALLOCAs contains the stack def.
+    InstrNum = {Num, 2};
----------------
super nit: I think the [[ https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements | style  guide prefers ]] braces around a single statement if if there is also a nested comment?


================
Comment at: llvm/lib/Target/X86/X86FrameLowering.cpp:1050
+      MF.makeDebugValueSubstitution(
+          *InstrNum, {ModInst->getDebugInstrNum(), NumCallOps - 1});
+    }
----------------
nits: Do you need `NumCallOps`? Is it not the same as `ModInst->getNumOperands()` here?

And since `ModInst == CI` on this code path (right?) IMO it would be clearer to refer to `CI` directly here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114452