[PATCH] D73943: [WebAssembly] Make stack pointer args inhibit tail calls

[Thomas Lively via Phabricator via llvm-commits]

tlively marked an inline comment as done.
tlively added a comment.

In D73943#1858326 <https://reviews.llvm.org/D73943#1858326>, @dschuff wrote:

> The langref says (https://llvm.org/docs/LangRef.html#call-instruction) that the tail call marker implies that the callee does not access allocas from the caller. So it seems like it *should* mean that the backend can depend on this property (that you're checking for here).  It also means that the frontend should guarantee it as best it knows how, and optimizations should not introduce it (or remove the attribute if they do?)


I agree with your interpretation, but there doesn't seem to be any validation that the callee doesn't access allocas of the caller, so this change makes the backend more robust in the presence of incorrect IR that nonetheless passes IR validation. That seems like a useful but non-essential property to have, so I would be ok not merging this except that I don't think there are any downsides.

> There are lots of ways to sneak pointers into places (aliasing, going through memory, etc etc) so I'd expect the check in this CL to be brittle.

The check does as much as it can, but you're right that it can't look through operations that lose type information. That means it is possible to generate IR that hides its reference to caller allocas in its tail call, but it's ok if we sometimes generate bad code in that case because the LangRef said you weren't supposed to do that.



================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:738
+          Val = Src;
+        }
+        if (isa<AllocaInst>(Val)) {
----------------
aheejin wrote:
> I think there can be other cases other than geps and aliases; looking at [[ https://github.com/llvm/llvm-project/blob/0c3b2986ac6b71abc649811c3ec9cb0bf073c7d8/llvm/lib/IR/Value.cpp#L466-L527 | this function ]] might give some ideas. Skimming at the code, it looks [[ https://github.com/llvm/llvm-project/blob/0c3b2986ac6b71abc649811c3ec9cb0bf073c7d8/llvm/lib/IR/Value.cpp#L605-L607 | `stripPointerCastsAndOffsets` ]] does the most comprehensive checking we need.
`stripPointerCastsAndOffsets` doesn't look through aliases, so it doesn't replace `stripPointerCastsAndAliases`. I could use both in the loop, but I think manually fetching the GEP pointer is clearer. I could probably be convinced otherwise, though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73943