<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<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">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">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>