[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 15:03:58 PDT 2021
Juneyoung,
As a gesture of good faith, I took a shot at reworking your change into
something which doesn't contradict our long standing implementation.
Please take a look at https://reviews.llvm.org/D99303.
I ask that either a) the above (or something analogous) lands, or b) you
revert your change by the end of the week. I tried to selectively
revert, and had trouble getting something sensible. Given that, if we
can't fix this ASAP, I do ask that you revert your entire change and
start again. What's currently in tree is inconsistent with our actual
implementation and that can not be allowed to continue.
If by Saturday, this is not resolved, I will revert the patches in
question.
Philip
p.s. Some of this is really subtle. I encourage you to setup an offline
call to discuss if anything in the discussion is unclear to you. Please
email directly if desired.
On 3/24/21 8:48 AM, Philip Reames via llvm-commits wrote:
>
> 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>
>>>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> 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/17806775/attachment-0001.html>
More information about the llvm-commits
mailing list