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