[llvm] [TailCallElim] Do not mark a call tail when it is handed the frame (PR #218797)

[Matt Turner via llvm-commits]

================
@@ -181,6 +181,14 @@ struct AllocaDerivedValueTracker {
       case Instruction::Call:
       case Instruction::Invoke: {
         auto &CB = cast<CallBase>(*I);
+        // llvm.stackrestore only assigns its argument to the stack pointer. It
+        // neither captures the value nor hands it to anything that could, so it
+        // is not an escape even though it writes memory and its argument is not
+        // marked nocapture. Without this every call after a VLA scope or a
+        // __builtin_stack_save would lose its tail marking.
+        if (auto *II = dyn_cast<IntrinsicInst>(I);
+            II && II->getIntrinsicID() == Intrinsic::stackrestore)
+          continue;
----------------
mattst88 wrote:

The attribute is the better factoring, and it works mechanically: `int_stackrestore` has no argument attributes today, so adding `NoCapture<ArgIndex<0>>` makes `doesNotCapture` true and `walk()` stops there without touching `EscapePoints`. This special case then goes away. (The older `stackrestore` bail further down is about mod/ref on unescaped dynamic allocas, not capture, so it stays either way.)

What I'm unsure of is whether `captures(none)` is honest here. LangRef says an argument that doesn't capture provenance makes accesses based on it after the call returns UB -- and `stackrestore` writes the value to the stack pointer, so every stack access after it is based on that value. Read literally, the attribute asserts the opposite of what the intrinsic does. Against that: LangRef already calls the `stacksave` result "an opaque pointer value", and the definition's comment says it's `writemem` only "because we don't otherwise model their dependencies on allocas", so the modelling is loose on purpose.

@efriedma-quic, thoughts? The attribute is global, so it also changes the answer for capture tracking on any pointer whose only use is `stackrestore`. That's always a `stacksave` result in practice, and BasicAA already special-cases dynamic allocas against `stackrestore`, so I'd expect it to be inert -- but I'd rather not assert that without your read.

Either way it doesn't close the store-to-slot gap from the PR summary: that escapes at the `store` before `stackrestore` is reached.

Happy to respin with the attribute, or leave the local check with a FIXME pointing at it.

https://github.com/llvm/llvm-project/pull/218797