[PATCH] D93376: [LangRef] Clarify the semantics of lifetime intrinsics

Fri Dec 18 10:59:58 PST 2020

jdoerfert added a comment.

In D93376#2463386 <https://reviews.llvm.org/D93376#2463386>, @nlopes wrote:

> As Ralf mentioned, the ship has sailed. Alloca and lifetime intrinsics were implemented like this several years ago. They were a quick hack to save stack space after inlining. That's it, and their design reflects the goals at the time.
> We simply want to document what is implemented. @jdoerfert you seem to want to change the implementation and/or the design, which is a separate discussion. I suggest we first document how LLVM works and then if you want to make changes you start a *separate* discussion on the things you want to change, why, and what's the upgrade path, etc. We can't change the semantics of either alloca or lifetime intrinsics without an automatic upgrade path as otherwise we would break all frontends out there.

But you don't "simply document what is implemeneted". You make up new *syntactic* restrictions tailored towards the use case in the backend. This changes the semantics of the IR. Now you somehow seem to argue that this is necessary to fix a bug and therefore we don't need a separate discussion for such a change (or the one you had is sufficient). I disagree.

Also, I did not suggest to change the semantics without an RFC and everything else that is needed. You suggest otherwise in your response. I did propose alternatives to fix your bug, but most of my responses show how the restrictions you want to put in place are making (potentially existing) optimizations invalid.

Finally, in my very first response I asked to split this into the clarifications and the additional restrictions. Since then we argued almost exclusively why those restrictions might be needed or why not. I am not convinced that the clarifications, in addition to the `isa<AllocaInst>(V->stripPointerCast())` at the use side, are not sufficient to fix your problem anyway.
At least I have not seen an example where the syntactic requirement was really needed.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93376