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

Mon Dec 21 21:52:15 PST 2020

nhaehnle added a comment.

>> 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.
>
> The thing is, in our view (aqjune, nlopes, and me) you *are* changing the semantics by allowing the lifetime intrinsics to be used on things like "malloc". From all we can tell, the intrinsics were never meant to be used with anything but alloca, so for all intents and purposes, currently, they only support alloca.

This isn't a very useful part of the discussion, but I'd say that as a matter of policy the LangRef must be a document that people can rely on, else what's the point of having it? Since it is written as-if lifetime intrinsics could be applied rather freely, I agree with @jdoerfert here. It's fair to say that this patch is a change of semantics by forbidding the use of the lifetime intrinsics in certain places where they were previously allowed. This is also reflected in the verifier changes that are part of the patch. Those changes should be justified on their own merit instead of pretending that they're not changes :)

That said, I find the direction of this patch very reasonable. The meaning of lifetime intrinsics has always been somewhat unclear to me, and this patch clarifies it in a way that makes sense: they have a pragmatic purpose specifically to enable efficient stack slot allocation in large functions. Preventing other uses can help us reduce the potential for confusion, which is a good thing.

>> Given the proposed semantic of lifetime.start, the program had UB semantics prior to inlining, we might not have known it statically but that is not an issue.
>
> The question is, *how* is it UB? To define UB precisely, we need an operational specification -- something that could, in principle, be implemented in a (complicated) interpreter. But for this example, which is the instruction that raises the UB? It cannot be the alloca, since when the alloca is being executed we have no way to know whether the return value will be used in a lifetime.start in the future! So does UB only arise when the lifetime.start happens? That would mean that the store itself is fine, but when lifetime.start happens we check if there was a load/store already and we raise UB at that point. I am not sure if that has the right behavior.

I agree that this is a bit iffy, though I'm not sure a literally operational specification is always required. Recall the example program:

  p = alloca i32
  store 0, p // analysis will say this is perfectly fine
  f(p) // lifetime.start(p) is hidden in f(p)

With this patch, the program isn't just UB, it actually becomes ill-formed IR because the lifetime.start in `f(p)` doesn't syntactically reference an alloca. So the example does not exist.

@jdoerfert's proposal is on point conceptually:

> 1. As you noted, there are "constrainted" and "unconstrained" allocas; if there might be a lifetime marker use, an alloca is constrained.
> 2. Once an alloca is unconstrained, it cannot go back, thus we need to manifest it with the alloca.
> 3. We do not know that constrained allocas have unique addresses, thus don't fold comparisons of (different) allocas if one is constrained.

The one change I would make in that proposal is to say that "if there **is** a lifetime marker use, an alloca is constrained". As long as the markers must be in the same IR function as the alloca itself, this determination can be made reliably (if not easily, due to the bitcast mess).

Ideally, there would be a flag on the alloca instruction itself to distinguish between constrained and unconstrained. As it is, the flag is implicitly given as "has a (indirect) lifetime.start use". That's not //great//, and perhaps you could make a follow-up patch that introduces such a flag explicitly, but I think it's workable either way.

I would suggest that you add language to the LangRef to make the distinction between constrained and unconstrained explicit, including the example of how it affects folding of pointer comparisons.

================
Comment at: llvm/docs/LangRef.rst:17647
+to the object. It should be either :ref:`alloca <i_alloca>` or a bitcast
+to :ref:`alloca <i_alloca>`.

----------------
nikic wrote:
> or a zero-index GEP, because we canoncalize bitcasts to that.
Assuming this language is agreed upon, replace "should" by "must". It's clearer, and this patch adds a corresponding check to the verifier.

================
Comment at: llvm/docs/LangRef.rst:17674-17675
+'``llvm.lifetime.start``' cannot relocate the alloca. In other words,
+observation of the address of an alloca during different lifetimes should
+yield the same value.
+
----------------
Make this a "must".

================
Comment at: llvm/docs/LangRef.rst:17701-17702
 object, or -1 if it is variable sized. The second argument is a pointer
-to the object.
+to the object. It should be either :ref:`alloca <i_alloca>` or a bitcast
+to :ref:`alloca <i_alloca>`.

----------------
Same here: "must" instead of "should"

================
Comment at: llvm/lib/IR/Verifier.cpp:5184
+           "bitcast to alloca.",
+           &Call);
+    break;
----------------
jdoerfert wrote:
> I'm doubtful of a strict syntactic requirement here. Using something from the  stripPointerCast family makes more sense to me.
I think the strict syntactic requirement is necessary to make this work without a "constrained vs. unconstrained" flag on the alloca instruction.

I agree that this probably needs stripPointerCast or similar. I also think that the two cases should be unified -- as it is, there's pointless code duplication.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93376