[PATCH] D114443: [DebugInfo][InstrRef] Cope with win32 calls changing the stack-pointer in LiveDebugValues

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

Orlando accepted this revision.
Orlando added a comment.
This revision is now accepted and ready to land.

I'm definitely not a windows expert by any stretch, so wouldn't object to a second opinion on this and the follow-up patch. But everything you've said makes sense and the changes LGTM (with a few inline nits).

> [0] This isn't true of stdcall, but I think we have existing difficulties with that anyway.

😬



================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:1301-1309
+  // We always ignore SP defines on call instructions, they don't actually
+  // change the value of the stack pointer... except for win32's _chkstk. This
+  // is rare: filter quickly for the common case (no stack adjustments, not a
+  // call, etc). If it is a call that modifies SP, recognise the SP register
+  // defs.
+  bool CallChangesSP = false;
+  if (AdjustsStackInCalls && MI.isCall() && MI.getOperand(0).isSymbol() &&
----------------
Any reason not to stuff this all inside the `IgnoreSPAlias` lambda? Not a strong opinion.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.h:822
+  /// normally assume that they don't clobber SP, but stack probes on Windows
+  /// does.
+  bool AdjustsStackInCalls = false;
----------------
nit: does- > do


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.h:825
+
+  /// If AdjustsStackInCalls is true, this holds the name of the targets stack
+  /// probe function, which is the function we expect will alter the stack
----------------
nit: target's


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114443