[llvm] c821ef4 - [LangRef] Make lifetime intrinsic's semantics consistent with StackColoring's comment
Philip Reames via llvm-commits
llvm-commits at lists.llvm.org
Tue Mar 23 09:38:12 PDT 2021
Juneyong,
Sorry for not replying sooner. Point 3 came up in the context of
https://reviews.llvm.org/D99135. I need to ask that you revert that
particular wording. More discussion is needed, and the current
semantics in the LangRef is problematic for dereferenceability. (Both
current and proposed semantics.)
Some detailed comments follow, but please don't get lost in the
discussion. The primary point of this email is the request in the
previous paragraph.
Philip
On 3/4/21 7:46 PM, Juneyoung Lee wrote:
> Hello Philip,
>
> On Fri, Mar 5, 2021 at 2:48 AM Philip Reames
> <listmail at philipreames.com <mailto:listmail at philipreames.com>> wrote:
>
> I see a couple of problems with the drafting on this that I think
> warrant tweaking.
>
> 1) The original wording for lifetime.start and lifetime.end applied
> equally to all memory objects, not just stack objects. I think
> it's a
> mistake to weaken them so much for non-stack objects. Glancing
> through
> the optimizer, I can't find a case where this actively causes a
> problem,
> but the lack of symmetry in the definition seems undesirable.
>
>
> There were discussions about allowing non-stack objects in llvm-dev's
> lifetime thread and the old lifetime patch
> (the threads are super long), and two issues were found:
>
> 1. AddressSanitizer may turn allocas into special allocator function
> calls without dropping their lifetime uses.
> 2. Since LangRef had been allowing lifetime with non-stack objects,
> changing it can turn programs that use
> lifetime with non-stack objects into an invalid program.
Why is this anything other than a bug in asan?
>
> To resolve 1, simply dropping the lifetime would work; for 2, we asked
> llvm-dev whether there is any such program language
> that needs the feature, but didn't get any reply.
I agree, I think the current semantics of lifetime end are badly
specified and a result, largely unusable by any frontend.
> But, we chose to be conservative here because this direction still
> requires changes in the implementations (either in LLVM
> or frontends that we're not aware of).
We also need to be considerate of the "defacto" semantics of the
intrinsics. We've had an in tree implementation for years. What the
optimizer has done, is, defacto, the actual semantics. (See point below
for further expansion on this.)
> Instead, the semantics w.r.t non-stack object is clarified so that the
> object explicitly contains poison bytes.
>
> It would be great if something that definitely requires excluding
> non-stack object lifetime uses is reported; this will allow us to make
> a further progress.
>
> 2) In the wording for lifetime.start, "If the offset is non-zero" is
> very unclear as to meaning. I think you mean that the pointer
> passed to
> the object is not the base of the memory object. At a minimum, this
> should be clarified. I think we can also make this case immediate
> UB,
> but am not going to argue strongly for it.
>
>
> Thanks for the feedback, I'll clarify it. :)
> I think lifetime.start/end shouldn't raise immediate UB because it is
> common for the intrinsic calls to be
> code-motioned.
Thanks for clarifying. The new wording is more clear.
>
> 3) The meaning of "dead" changed in this revision in a problematic
> way.
> The original wording said that loads from dead objects were
> undef. The
> new wording says that accessing a dead object is immediate UB.
> From the
> commit comment, this doesn't appear to have been an intentional
> change.
> I ask that this be reversed and reviewed separately if actually
> desired. If nothing else, there's a backwards compatibility
> concern here.
>
>
> The change was intentional because the comment at StackColoring.cpp says
> memory accesses to out-of-scope stack slots are UB.
> Also, the original wording was unclear as well - it says loads from
> dead objects can be *optimized to*
> undef, implying it could be immediate UB. The new wording is a
> clarification for this.
>
> I think this change based on StackColoring.cpp's comment is okay: it
> was the only place where
> the meaning of lifetime was clearly mentioned, and also found that
> frontend language developers
> were also relying on the comment.
I went and I read the StackColoring.cpp comments. I see where you're
coming from, but I think the definition used there is problematic. I
also suspect you're reading more specificity into the wording than was
intended by the patch author. :)
At a macro level, you need to consider and balance the implementation of
more than StackColoring. As an example, consider LICM. Today, LICM
does not consider lifetime end to effect the speculation safety of
reordering a load. Consider this example:
(assume obj is an alloca)
if (c) { lifetime_end(obj) }
loop {
if (!c)
v = obj->f;
.. body ..
}
if (!c) { lifetime_end(obj) }
LICM will convert this to:
if (c) { lifetime_end(obj) }
v_1 = obj->f;
loop {
if (!c)
v = v_1;
.. body ..
}
if (!c) { lifetime_end(obj) }
This is a desirable transform. We do not wish to disallow this.
With the semantics you added to the LangRef, this is now introducing UB
into the program.
Please revert that particular sentence. The rest of your change can
stand, but that point is actively problematic and needs removed.
> But if it still requires further discussion, I'll temporarily revert
> relevant sentences and open a Phabricator patch for this.
> Juneyoung
>
> On 3/3/21 4:58 PM, Juneyoung Lee via llvm-commits wrote:
> > Author: Juneyoung Lee
> > Date: 2021-03-04T09:58:06+09:00
> > New Revision: c821ef451373849c403686042466d30a7bf8138a
> >
> > URL:
> https://github.com/llvm/llvm-project/commit/c821ef451373849c403686042466d30a7bf8138a
> <https://github.com/llvm/llvm-project/commit/c821ef451373849c403686042466d30a7bf8138a>
> > DIFF:
> https://github.com/llvm/llvm-project/commit/c821ef451373849c403686042466d30a7bf8138a.diff
> <https://github.com/llvm/llvm-project/commit/c821ef451373849c403686042466d30a7bf8138a.diff>
> >
> > LOG: [LangRef] Make lifetime intrinsic's semantics consistent
> with StackColoring's comment
> >
> > This patch is an update to LangRef by describing lifetime
> intrinsics' behavior
> > by following the description of MIR's
> LIFETIME_START/LIFETIME_END markers
> > at StackColoring.cpp
> (https://github.com/llvm/llvm-project/blob/eb44682d671d66e422b02595a636050582a4d84a/llvm/lib/CodeGen/StackColoring.cpp#L163
> <https://github.com/llvm/llvm-project/blob/eb44682d671d66e422b02595a636050582a4d84a/llvm/lib/CodeGen/StackColoring.cpp#L163>)
> and the discussion in llvm-dev.
> >
> > In order to explicitly define the meaning of an object lifetime,
> I added 'Object Lifetime' subsection.
> >
> > Reviewed By: nlopes
> >
> > Differential Revision: https://reviews.llvm.org/D94002
> <https://reviews.llvm.org/D94002>
> >
> > Added:
> >
> >
> > Modified:
> > llvm/docs/LangRef.rst
> >
> > Removed:
> >
> >
> >
> >
> ################################################################################
> > diff --git a/llvm/docs/LangRef.rst b/llvm/docs/LangRef.rst
> > index cc2fcef30831..6866cc362f57 100644
> > --- a/llvm/docs/LangRef.rst
> > +++ b/llvm/docs/LangRef.rst
> > @@ -2544,6 +2544,33 @@ This information is passed along to the
> backend so that it generates
> > code for the proper architecture. It's possible to override
> this on the
> > command line with the ``-mtriple`` command line option.
> >
> > +.. _objectlifetime:
> > +
> > +Object Lifetime
> > +----------------------
> > +
> > +A memory object, or simply object, is a region of a memory
> space that is
> > +reserved by a memory allocation such as :ref:`alloca
> <i_alloca>`, heap
> > +allocation calls, and global variable definitions.
> > +Once it is allocated, the bytes stored in the region can only
> be read or written
> > +through a pointer that is :ref:`based on <_pointeraliasing>`
> the allocation
> > +value.
> > +If a pointer that is not based on the object tries to read or
> write to the
> > +object, it is undefined behavior.
> > +
> > +A lifetime of a memory object is a property that decides its
> accessibility.
> > +Unless stated otherwise, a memory object is alive since its
> allocation, and
> > +dead after its deallocation.
> > +It is undefined behavior to access a memory object that isn't
> alive, but
> > +operations that don't dereference it such as
> > +:ref:`getelementptr <i_getelementptr>`, :ref:`ptrtoint
> <i_ptrtoint>` and
> > +:ref:`icmp <i_icmp>` return a valid result.
> > +This explains code motion of these instructions across
> operations that
> > +impact the object's lifetime.
> > +A stack object's lifetime can be explicitly specified using
> > +:ref:`llvm.lifetime.start <_int_lifestart>` and
> > +:ref:`llvm.lifetime.end <_int_lifeend>` intrinsic function calls.
> > +
> > .. _pointeraliasing:
> >
> > Pointer Aliasing Rules
> > @@ -17808,8 +17835,9 @@ Other targets may support this intrinsic
> > diff erently, for example, by lowering i
> > Memory Use Markers
> > ------------------
> >
> > -This class of intrinsics provides information about the lifetime of
> > -memory objects and ranges where variables are immutable.
> > +This class of intrinsics provides information about the
> > +:ref:`lifetime of memory objects <_objectlifetime>` and ranges
> where variables
> > +are immutable.
> >
> > .. _int_lifestart:
> >
> > @@ -17826,8 +17854,8 @@ Syntax:
> > Overview:
> > """""""""
> >
> > -The '``llvm.lifetime.start``' intrinsic specifies the start of
> a memory
> > -object's lifetime.
> > +The '``llvm.lifetime.start``' intrinsic specifies the start of
> > +:ref:`a memory object's lifetime <_objectlifetime>`.
> >
> > Arguments:
> > """"""""""
> > @@ -17839,10 +17867,23 @@ to the object.
> > Semantics:
> > """"""""""
> >
> > -This intrinsic indicates that before this point in the code,
> the value
> > -of the memory pointed to by ``ptr`` is dead. This means that it
> is known
> > -to never be used and has an undefined value. A load from the
> pointer
> > -that precedes this intrinsic can be replaced with ``'undef'``.
> > +If ``ptr`` is a stack-allocated object and its offset is zero,
> the object is
> > +initially marked as dead.
> > +After '``llvm.lifetime.start``', the stack object that ``ptr``
> points is marked
> > +as alive and has an uninitialized value.
> > +The stack object is marked as dead when either
> > +:ref:`llvm.lifetime.end <int_lifeend>` to the alloca is
> executed or the
> > +function returns.
> > +
> > +After :ref:`llvm.lifetime.end <int_lifeend>` is called,
> > +'``llvm.lifetime.start``' on the stack object can be called again.
> > +The second '``llvm.lifetime.start``' call marks the object as
> alive, but it
> > +does not change the address of the object.
> > +
> > +If ``ptr`` is a non-stack-allocated object, its offset is
> non-zero or it is
> > +a stack object that is already alive, it simply fills all bytes
> of the object
> > +with ``poison``.
> > +
> >
> > .. _int_lifeend:
> >
> > @@ -17859,8 +17900,8 @@ Syntax:
> > Overview:
> > """""""""
> >
> > -The '``llvm.lifetime.end``' intrinsic specifies the end of a memory
> > -object's lifetime.
> > +The '``llvm.lifetime.end``' intrinsic specifies the end of
> > +:ref:`a memory object's lifetime <_objectlifetime>`.
> >
> > Arguments:
> > """"""""""
> > @@ -17872,10 +17913,13 @@ to the object.
> > Semantics:
> > """"""""""
> >
> > -This intrinsic indicates that after this point in the code, the
> value of
> > -the memory pointed to by ``ptr`` is dead. This means that it is
> known to
> > -never be used and has an undefined value. Any stores into the
> memory
> > -object following this intrinsic may be removed as dead.
> > +If ``ptr`` is a stack-allocated object and its offset is zero,
> the object is
> > +dead.
> > +Calling ``llvm.lifetime.end`` on an already dead alloca is no-op.
> > +
> > +If ``ptr`` is a non-stack-allocated object or its offset is
> non-zero,
> > +it is equivalent to simply filling all bytes of the object with
> ``poison``.
> > +
> >
> > '``llvm.invariant.start``' Intrinsic
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >
> >
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>
> > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> <https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210323/939421f4/attachment.html>
More information about the llvm-commits
mailing list