[llvm] c821ef4 - [LangRef] Make lifetime intrinsic's semantics consistent with StackColoring's comment
Philip Reames via llvm-commits
llvm-commits at lists.llvm.org
Wed Mar 24 08:48:57 PDT 2021
Juneyoung,
I have raised post commit review feedback on a review and have asked for
a revert. I agree that we should move the discussion to a new review.
That responsibility lies with you once the problematic piece of the
change has been reverted. Please revert.
Philip
On 3/24/21 5:26 AM, Juneyoung Lee wrote:
> Hi,
>
> Would defining load on a dead alloca as non-UB and simply returning
> poison instead allow the LICM transformation?
> Then, I think it is better to open a new Phabricator patch that makes
> the change to LangRef and discuss there.
>
> p.s: I'm slightly in hardcore mode this week, so my reply can be a bit
> late :(
>
> On Wed, Mar 24, 2021 at 1:38 AM Philip Reames
> <listmail at philipreames.com <mailto:listmail at philipreames.com>> wrote:
>
> Juneyong,
>
> Sorry for not replying sooner. Point 3 came up in the context of
> https://reviews.llvm.org/D99135 <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/20210324/c5bc88f2/attachment-0001.html>
More information about the llvm-commits
mailing list