<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<p>I am retracting my insistence that the changes be reverted or
revised. I still think they should be, but I will no longer
insist on that.</p>
<p>I have received several bits of feedback that my handling of this
thread and a couple of other recent ones is too aggressive. At a
certain point, it's time to listen when you're being told by
multiple parties you're the one in the wrong.</p>
<p>This is also no longer a practically blocking item for me. My
level of concern on the wording problems pointed out in the
original response increased sharply when that wording was recently
used to in the discussion of a seemingly (to me) unrelated review
(D99135). That particular discussion point has been resolved, and
(I think) the root cause of the miscommunication addressed. As
such, this is no longer an acute issue for me. <br>
</p>
<p>Given all of that, I think it's time I removed myself from this
discussion. Juneyoung, I will respond on the technical point you
raised in review (D99303), but after that I will try to refrain
from commenting in this area again for the near future. <br>
</p>
Philip<br>
<p>On 3/24/21 3:03 PM, Philip Reames via llvm-commits wrote:</p>
<blockquote type="cite"
cite="mid:153cce08-cf38-8779-e45e-1dc5c6fc9069@philipreames.com">
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
<p>Juneyoung,</p>
<p>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 <a
class="moz-txt-link-freetext"
href="https://reviews.llvm.org/D99303" moz-do-not-send="true">https://reviews.llvm.org/D99303</a>.</p>
<p>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. </p>
<p>If by Saturday, this is not resolved, I will revert the patches
in question. <br>
</p>
<p>Philip</p>
<p>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. <br>
</p>
<div class="moz-cite-prefix">On 3/24/21 8:48 AM, Philip Reames via
llvm-commits wrote:<br>
</div>
<blockquote type="cite"
cite="mid:68b9f51d-6aad-482a-62e7-8a6968a8e4b7@philipreames.com">
<meta http-equiv="Content-Type" content="text/html;
charset=UTF-8">
<p>Juneyoung,</p>
<p>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. <br>
</p>
<p>Philip<br>
</p>
<div class="moz-cite-prefix">On 3/24/21 5:26 AM, Juneyoung Lee
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CAJOWn3jepd3s8vePHY8sS1d8+5XwjjEPy0bvJWWQV5DiF-wtYw@mail.gmail.com">
<meta http-equiv="content-type" content="text/html;
charset=UTF-8">
<div dir="ltr">Hi,
<div><br>
</div>
<div>Would defining load on a dead alloca as non-UB and
simply returning poison instead allow the LICM
transformation?</div>
<div>Then, I think it is better to open a new Phabricator
patch that makes the change to LangRef and discuss there.</div>
<div><br>
</div>
<div>p.s: I'm slightly in hardcore mode this week, so my
reply can be a bit late :(</div>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Wed, Mar 24, 2021 at
1:38 AM Philip Reames <<a
href="mailto:listmail@philipreames.com"
moz-do-not-send="true">listmail@philipreames.com</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">
<div>
<p>Juneyong,</p>
<p>Sorry for not replying sooner. Point 3 came up in
the context of <a
href="https://reviews.llvm.org/D99135"
target="_blank" moz-do-not-send="true">https://reviews.llvm.org/D99135</a>.
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.)</p>
<p>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. <br>
</p>
<p>Philip<br>
</p>
<div>On 3/4/21 7:46 PM, Juneyoung Lee wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">
<div dir="ltr">
<div>Hello Philip,</div>
<br>
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Fri, Mar 5,
2021 at 2:48 AM Philip Reames <<a
href="mailto:listmail@philipreames.com"
target="_blank" moz-do-not-send="true">listmail@philipreames.com</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote"
style="margin:0px 0px 0px
0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">I see a
couple of problems with the drafting on this
that I think <br>
warrant tweaking.<br>
<br>
1) The original wording for lifetime.start and
lifetime.end applied <br>
equally to all memory objects, not just stack
objects. I think it's a <br>
mistake to weaken them so much for non-stack
objects. Glancing through <br>
the optimizer, I can't find a case where this
actively causes a problem, <br>
but the lack of symmetry in the definition
seems undesirable.<br>
</blockquote>
<div><br>
</div>
<div>There were discussions about allowing
non-stack objects in llvm-dev's lifetime
thread and the old lifetime patch</div>
<div>(the threads are super long), and two
issues were found:</div>
<div><br>
</div>
<div>1. AddressSanitizer may turn allocas into
special allocator function calls without
dropping their lifetime uses.</div>
<div>2. Since LangRef had been allowing lifetime
with non-stack objects, changing it can turn
programs that use</div>
<div>lifetime with non-stack objects into an
invalid program.</div>
</div>
</div>
</div>
</blockquote>
Why is this anything other than a bug in asan? <br>
<blockquote type="cite">
<div dir="ltr">
<div dir="ltr">
<div class="gmail_quote">
<div><br>
</div>
<div>To resolve 1, simply dropping the lifetime
would work; for 2, we asked llvm-dev whether
there is any such program language</div>
<div>that needs the feature, but didn't get any
reply.</div>
</div>
</div>
</div>
</blockquote>
I agree, I think the current semantics of lifetime end
are badly specified and a result, largely unusable by
any frontend. <br>
<blockquote type="cite">
<div dir="ltr">
<div dir="ltr">
<div class="gmail_quote">
<div>But, we chose to be conservative here
because this direction still requires changes
in the implementations (either in LLVM</div>
<div>or frontends that we're not aware of).</div>
</div>
</div>
</div>
</blockquote>
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.)<br>
<blockquote type="cite">
<div dir="ltr">
<div dir="ltr">
<div class="gmail_quote">
<div>Instead, the semantics w.r.t non-stack
object is clarified so that the object
explicitly contains poison bytes.</div>
<div><br>
</div>
<div>It would be great if something that
definitely requires excluding non-stack object
lifetime uses is reported; this will allow us
to make</div>
<div>a further progress.</div>
<div> <br>
</div>
<blockquote class="gmail_quote"
style="margin:0px 0px 0px
0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">2) In the
wording for lifetime.start, "If the offset is
non-zero" is <br>
very unclear as to meaning. I think you mean
that the pointer passed to <br>
the object is not the base of the memory
object. At a minimum, this <br>
should be clarified. I think we can also make
this case immediate UB, <br>
but am not going to argue strongly for it.<br>
</blockquote>
<div><br>
</div>
<div>Thanks for the feedback, I'll clarify it.
:)</div>
<div>I think lifetime.start/end shouldn't raise
immediate UB because it is common for the
intrinsic calls to be</div>
<div>code-motioned.</div>
</div>
</div>
</div>
</blockquote>
Thanks for clarifying. The new wording is more clear.
<br>
<blockquote type="cite">
<div dir="ltr">
<div dir="ltr">
<div class="gmail_quote">
<div> </div>
<blockquote class="gmail_quote"
style="margin:0px 0px 0px
0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">3) The
meaning of "dead" changed in this revision in
a problematic way. <br>
The original wording said that loads from dead
objects were undef. The <br>
new wording says that accessing a dead object
is immediate UB. From the <br>
commit comment, this doesn't appear to have
been an intentional change. <br>
I ask that this be reversed and reviewed
separately if actually <br>
desired. If nothing else, there's a backwards
compatibility concern here.<br>
</blockquote>
<div><br>
</div>
<div>The change was intentional because the
comment at StackColoring.cpp says</div>
<div>memory accesses to out-of-scope stack slots
are UB.</div>
<div>Also, the original wording was unclear as
well - it says loads from dead objects can be
*optimized to*</div>
<div>undef, implying it could be immediate UB.
The new wording is a clarification for this.</div>
<div><br>
</div>
<div>I think this change based on
StackColoring.cpp's comment is okay: it was
the only place where</div>
<div>the meaning of lifetime was clearly
mentioned, and also found that frontend
language developers</div>
<div>were also relying on the comment.</div>
</div>
</div>
</div>
</blockquote>
<p>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. :)</p>
<p>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:</p>
<p>(assume obj is an alloca)<br>
if (c) { lifetime_end(obj) }<br>
loop {<br>
if (!c) <br>
v = obj->f;<br>
.. body .. <br>
}<br>
if (!c) { lifetime_end(obj) }<br>
</p>
<p>LICM will convert this to:<br>
</p>
<p>if (c) { lifetime_end(obj) }<br>
v_1 = obj->f;<br>
loop {<br>
if (!c) <br>
v = v_1;<br>
.. body .. <br>
}<br>
if (!c) { lifetime_end(obj) }</p>
<p>This is a desirable transform. We do not wish to
disallow this.<br>
</p>
<p>With the semantics you added to the LangRef, this is
now introducing UB into the program.</p>
<p>Please revert that particular sentence. The rest of
your change can stand, but that point is actively
problematic and needs removed. <br>
</p>
<p><br>
</p>
<p><br>
</p>
<blockquote type="cite">
<div dir="ltr">
<div dir="ltr">
<div class="gmail_quote">
<div>But if it still requires further
discussion, I'll temporarily revert relevant
sentences and open a Phabricator patch for
this.</div>
<div> </div>
<div>Juneyoung</div>
<div> </div>
<blockquote class="gmail_quote"
style="margin:0px 0px 0px
0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">On 3/3/21
4:58 PM, Juneyoung Lee via llvm-commits wrote:<br>
> Author: Juneyoung Lee<br>
> Date: 2021-03-04T09:58:06+09:00<br>
> New Revision:
c821ef451373849c403686042466d30a7bf8138a<br>
><br>
> URL: <a
href="https://github.com/llvm/llvm-project/commit/c821ef451373849c403686042466d30a7bf8138a"
rel="noreferrer" target="_blank"
moz-do-not-send="true">https://github.com/llvm/llvm-project/commit/c821ef451373849c403686042466d30a7bf8138a</a><br>
> DIFF: <a
href="https://github.com/llvm/llvm-project/commit/c821ef451373849c403686042466d30a7bf8138a.diff"
rel="noreferrer" target="_blank"
moz-do-not-send="true">https://github.com/llvm/llvm-project/commit/c821ef451373849c403686042466d30a7bf8138a.diff</a><br>
><br>
> LOG: [LangRef] Make lifetime intrinsic's
semantics consistent with StackColoring's
comment<br>
><br>
> This patch is an update to LangRef by
describing lifetime intrinsics' behavior<br>
> by following the description of MIR's
LIFETIME_START/LIFETIME_END markers<br>
> at StackColoring.cpp (<a
href="https://github.com/llvm/llvm-project/blob/eb44682d671d66e422b02595a636050582a4d84a/llvm/lib/CodeGen/StackColoring.cpp#L163"
rel="noreferrer" target="_blank"
moz-do-not-send="true">https://github.com/llvm/llvm-project/blob/eb44682d671d66e422b02595a636050582a4d84a/llvm/lib/CodeGen/StackColoring.cpp#L163</a>)
and the discussion in llvm-dev.<br>
><br>
> In order to explicitly define the meaning
of an object lifetime, I added 'Object
Lifetime' subsection.<br>
><br>
> Reviewed By: nlopes<br>
><br>
> Differential Revision: <a
href="https://reviews.llvm.org/D94002"
rel="noreferrer" target="_blank"
moz-do-not-send="true">https://reviews.llvm.org/D94002</a><br>
><br>
> Added:<br>
> <br>
><br>
> Modified:<br>
> llvm/docs/LangRef.rst<br>
><br>
> Removed:<br>
> <br>
><br>
><br>
>
################################################################################<br>
> diff --git a/llvm/docs/LangRef.rst
b/llvm/docs/LangRef.rst<br>
> index cc2fcef30831..6866cc362f57 100644<br>
> --- a/llvm/docs/LangRef.rst<br>
> +++ b/llvm/docs/LangRef.rst<br>
> @@ -2544,6 +2544,33 @@ This information
is passed along to the backend so that it
generates<br>
> code for the proper architecture. It's
possible to override this on the<br>
> command line with the ``-mtriple``
command line option.<br>
> <br>
> +.. _objectlifetime:<br>
> +<br>
> +Object Lifetime<br>
> +----------------------<br>
> +<br>
> +A memory object, or simply object, is a
region of a memory space that is<br>
> +reserved by a memory allocation such as
:ref:`alloca <i_alloca>`, heap<br>
> +allocation calls, and global variable
definitions.<br>
> +Once it is allocated, the bytes stored
in the region can only be read or written<br>
> +through a pointer that is :ref:`based on
<_pointeraliasing>` the allocation<br>
> +value.<br>
> +If a pointer that is not based on the
object tries to read or write to the<br>
> +object, it is undefined behavior.<br>
> +<br>
> +A lifetime of a memory object is a
property that decides its accessibility.<br>
> +Unless stated otherwise, a memory object
is alive since its allocation, and<br>
> +dead after its deallocation.<br>
> +It is undefined behavior to access a
memory object that isn't alive, but<br>
> +operations that don't dereference it
such as<br>
> +:ref:`getelementptr
<i_getelementptr>`, :ref:`ptrtoint
<i_ptrtoint>` and<br>
> +:ref:`icmp <i_icmp>` return a
valid result.<br>
> +This explains code motion of these
instructions across operations that<br>
> +impact the object's lifetime.<br>
> +A stack object's lifetime can be
explicitly specified using<br>
> +:ref:`llvm.lifetime.start
<_int_lifestart>` and<br>
> +:ref:`llvm.lifetime.end
<_int_lifeend>` intrinsic function
calls.<br>
> +<br>
> .. _pointeraliasing:<br>
> <br>
> Pointer Aliasing Rules<br>
> @@ -17808,8 +17835,9 @@ Other targets may
support this intrinsic<br>
> diff erently, for example, by lowering i<br>
> Memory Use Markers<br>
> ------------------<br>
> <br>
> -This class of intrinsics provides
information about the lifetime of<br>
> -memory objects and ranges where
variables are immutable.<br>
> +This class of intrinsics provides
information about the<br>
> +:ref:`lifetime of memory objects
<_objectlifetime>` and ranges where
variables<br>
> +are immutable.<br>
> <br>
> .. _int_lifestart:<br>
> <br>
> @@ -17826,8 +17854,8 @@ Syntax:<br>
> Overview:<br>
> """""""""<br>
> <br>
> -The '``llvm.lifetime.start``' intrinsic
specifies the start of a memory<br>
> -object's lifetime.<br>
> +The '``llvm.lifetime.start``' intrinsic
specifies the start of<br>
> +:ref:`a memory object's lifetime
<_objectlifetime>`.<br>
> <br>
> Arguments:<br>
> """"""""""<br>
> @@ -17839,10 +17867,23 @@ to the object.<br>
> Semantics:<br>
> """"""""""<br>
> <br>
> -This intrinsic indicates that before
this point in the code, the value<br>
> -of the memory pointed to by ``ptr`` is
dead. This means that it is known<br>
> -to never be used and has an undefined
value. A load from the pointer<br>
> -that precedes this intrinsic can be
replaced with ``'undef'``.<br>
> +If ``ptr`` is a stack-allocated object
and its offset is zero, the object is<br>
> +initially marked as dead.<br>
> +After '``llvm.lifetime.start``', the
stack object that ``ptr`` points is marked<br>
> +as alive and has an uninitialized value.<br>
> +The stack object is marked as dead when
either<br>
> +:ref:`llvm.lifetime.end
<int_lifeend>` to the alloca is executed
or the<br>
> +function returns.<br>
> +<br>
> +After :ref:`llvm.lifetime.end
<int_lifeend>` is called,<br>
> +'``llvm.lifetime.start``' on the stack
object can be called again.<br>
> +The second '``llvm.lifetime.start``'
call marks the object as alive, but it<br>
> +does not change the address of the
object.<br>
> +<br>
> +If ``ptr`` is a non-stack-allocated
object, its offset is non-zero or it is<br>
> +a stack object that is already alive, it
simply fills all bytes of the object<br>
> +with ``poison``.<br>
> +<br>
> <br>
> .. _int_lifeend:<br>
> <br>
> @@ -17859,8 +17900,8 @@ Syntax:<br>
> Overview:<br>
> """""""""<br>
> <br>
> -The '``llvm.lifetime.end``' intrinsic
specifies the end of a memory<br>
> -object's lifetime.<br>
> +The '``llvm.lifetime.end``' intrinsic
specifies the end of<br>
> +:ref:`a memory object's lifetime
<_objectlifetime>`.<br>
> <br>
> Arguments:<br>
> """"""""""<br>
> @@ -17872,10 +17913,13 @@ to the object.<br>
> Semantics:<br>
> """"""""""<br>
> <br>
> -This intrinsic indicates that after this
point in the code, the value of<br>
> -the memory pointed to by ``ptr`` is
dead. This means that it is known to<br>
> -never be used and has an undefined
value. Any stores into the memory<br>
> -object following this intrinsic may be
removed as dead.<br>
> +If ``ptr`` is a stack-allocated object
and its offset is zero, the object is<br>
> +dead.<br>
> +Calling ``llvm.lifetime.end`` on an
already dead alloca is no-op.<br>
> +<br>
> +If ``ptr`` is a non-stack-allocated
object or its offset is non-zero,<br>
> +it is equivalent to simply filling all
bytes of the object with ``poison``.<br>
> +<br>
> <br>
> '``llvm.invariant.start``' Intrinsic<br>
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^<br>
><br>
><br>
> <br>
>
_______________________________________________<br>
> llvm-commits mailing list<br>
> <a
href="mailto:llvm-commits@lists.llvm.org"
target="_blank" moz-do-not-send="true">llvm-commits@lists.llvm.org</a><br>
> <a
href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits"
rel="noreferrer" target="_blank"
moz-do-not-send="true">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote>
</div>
</div>
</div>
</blockquote>
</div>
</blockquote>
</div>
</blockquote>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<pre class="moz-quote-pre" wrap="">_______________________________________________
llvm-commits mailing list
<a class="moz-txt-link-abbreviated" href="mailto:llvm-commits@lists.llvm.org" moz-do-not-send="true">llvm-commits@lists.llvm.org</a>
<a class="moz-txt-link-freetext" href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" moz-do-not-send="true">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a>
</pre>
</blockquote>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<pre class="moz-quote-pre" wrap="">_______________________________________________
llvm-commits mailing list
<a class="moz-txt-link-abbreviated" href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>
<a class="moz-txt-link-freetext" href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a>
</pre>
</blockquote>
</body>
</html>