<div dir="ltr"><div dir="ltr">Thanks to all for feedback!<div><br></div><div>I updated the revision <a href="https://reviews.llvm.org/D118426">https://reviews.llvm.org/D118426</a> accordingly.</div><div><br></div><div><div dir="ltr" class="gmail_attr">On Fri, May 6, 2022 at 1:58 AM Philip Reames <<a href="mailto:listmail@philipreames.com">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"><p>The structure given in the patch is not idiomatic. The idiomatic piece would be:<br></p><p>if (sa<ConstrainedFPIntrinsic>(CI)) {<br> if (Value *V = SimplifyCall(&CI, SQ.getWithInstruction(&CI))) {<br> return IC.replaceAllUsesOf(CI, V);<br> }<br>}<br></p></blockquote></div><div>Using replaceAllUsesOf is not suitable here because it won't work with instructions without uses:</div><div><a href="https://github.com/llvm/llvm-project/blob/7dc3c6190ec7191dd104fa5158fe0ee32e9b0c49/llvm/lib/Transforms/InstCombine/InstCombineInternal.h#L407">https://github.com/llvm/llvm-project/blob/7dc3c6190ec7191dd104fa5158fe0ee32e9b0c49/llvm/lib/Transforms/InstCombine/InstCombineInternal.h#L407</a><br></div><div><br></div><div>Function InstCombiner::eraseInstFromFunction is widely used in IC, VisitCall already has several calls of it, so it should not be a problem to use it here.</div><div><br clear="all"><div><div dir="ltr" class="gmail_signature" data-smartmail="gmail_signature">Thanks,<br>--Serge<br></div></div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, May 6, 2022 at 2:11 AM Philip Reames via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</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><br>
</p>
<div>On 5/5/22 11:54, Eli Friedman wrote:<br>
</div>
<blockquote type="cite">
<div>
<p class="MsoNormal"><u></u> <u></u></p>
<div>
<div style="border-right:none;border-bottom:none;border-left:none;border-top:1pt solid rgb(225,225,225);padding:3pt 0in 0in">
<p class="MsoNormal" style="margin-left:0.5in"><b>From:</b>
llvm-commits <a href="mailto:llvm-commits-bounces@lists.llvm.org" target="_blank"><llvm-commits-bounces@lists.llvm.org></a>
<b>On Behalf Of </b>Nikita Popov via llvm-commits<br>
<b>Sent:</b> Thursday, May 5, 2022 11:44 AM<br>
<b>To:</b> Philip Reames <a href="mailto:listmail@philipreames.com" target="_blank"><listmail@philipreames.com></a><br>
<b>Cc:</b> Serge Pavlov <a href="mailto:llvmlistbot@llvm.org" target="_blank"><llvmlistbot@llvm.org></a>;
llvm-commits <a href="mailto:llvm-commits@lists.llvm.org" target="_blank"><llvm-commits@lists.llvm.org></a><br>
<b>Subject:</b> Re: [llvm] 83914ee - [InstCombine] Remove
side effect of replaced constrained intrinsics<u></u><u></u></p>
</div>
</div>
<p class="MsoNormal" style="margin-left:0.5in"><u></u> <u></u></p>
<p style="margin-left:0.5in;text-align:center" align="center"><strong><span style="font-size:10.5pt;font-family:Arial,sans-serif;color:black;background:yellow">WARNING:</span></strong><span style="font-size:10.5pt;font-family:Arial,sans-serif;color:black;background:yellow">
This email originated from outside of Qualcomm. Please be
wary of any links or attachments, and do not enable macros.</span><u></u><u></u></p>
<div>
<div>
<div>
<div>
<p class="MsoNormal" style="margin-left:0.5in">On Thu,
May 5, 2022 at 6:17 PM Philip Reames via llvm-commits
<<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>>
wrote:<u></u><u></u></p>
</div>
<blockquote style="border-top:none;border-right:none;border-bottom:none;border-left:1pt solid rgb(204,204,204);padding:0in 0in 0in 6pt;margin-left:4.8pt;margin-right:0in">
<div>
<p style="margin-left:0.5in"><u></u> <u></u></p>
<div>
<p class="MsoNormal" style="margin-left:0.5in">On
5/5/22 09:14, Serge Pavlov wrote:<u></u><u></u></p>
</div>
<blockquote style="margin-top:5pt;margin-bottom:5pt">
<div>
<div>
<blockquote style="border-top:none;border-right:none;border-bottom:none;border-left:1pt solid rgb(204,204,204);padding:0in 0in 0in 6pt;margin-left:4.8pt;margin-right:0in">
<p class="MsoNormal" style="margin-left:0.5in">This
looks incorrect to me. SimplifyCall returns
a Value* for a<br>
simplified expression and you're
interpreting that as a boolean meaning<br>
"delete call".<u></u><u></u></p>
</blockquote>
<div>
<p class="MsoNormal" style="margin-left:0.5in"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal" style="margin-left:0.5in">In
this case SimplifyCall is called for a call,
which is not used and was not removed only
because it has declared side effect.
Non-null replacement returned by
SimplifyCall is used as an indicator that
the call may be simplified, so its side
effect is absent or may be ignored. Such
unused calls appear as a result of other
transformations, they are not removed due to
the side effect and produce useless
instructions in resulting code.
<u></u><u></u></p>
</div>
</div>
</div>
</blockquote>
<p style="margin-left:0.5in">My point is that
SimplifyCall is allowed to simplify one side
effecting call to another side effecting call. As
in, you can't make assumptions about *which*
simplifications SimplifyCall has made. If it
returns a nonnull result, you must use it (or leave
the instruction unmodified). <u></u><u></u></p>
</div>
</blockquote>
<div>
<p class="MsoNormal" style="margin-left:0.5in">Hey
Philip,<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal" style="margin-left:0.5in"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal" style="margin-left:0.5in">This is
not quite how InstSimplify works: It returns a
refinement of the instruction return value. The
simplification result has no relation to modelling of
side effects.<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal" style="margin-left:0.5in"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal" style="margin-left:0.5in">For
example, let's say you have "%x = call i32 @foo(i32
returned %y)" then InstSimplify could simplify %x to
%y, because the return value of the call is known,
regardless of any side-effects it may have. Replacing
uses of %x with %y is legal, but removing the call is
not. (InstSimplify doesn't actually perform this fold
because of reasons related to CGSSC passes, but it
could perform it, and serves as a simple
illustration.)<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal" style="margin-left:0.5in"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal" style="margin-left:0.5in">Of course,
this also means that generally you can't say that
"because SimplifyCall returned something, you can drop
the call" -- that would be obviously illegal for the
aforementioned fold.<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal" style="margin-left:0.5in"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal" style="margin-left:0.5in">What this
patch is doing is to say that for constrained FP
intrinsics in particular, SimplifyCall gains an
additional contract that InstSimplify does not
generally have: It will only return a value if the
constrained FP intrinsic is side-effect free.<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal" style="margin-left:0.5in"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal" style="margin-left:0.5in">The reason
for this is pragmatic, as determining whether a
constrained FP call is side-effect free is essentially
the same as determining whether it can be folded. As
such, this patch tries to piggy-back on the existing
simplification logic to determine whether the call can
also be removed. This somewhat dirties the API
contract, but avoids the need to reimplement folding
logic in wouldInstructionBeTriviallyDead().<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal" style="margin-left:0.5in"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal" style="margin-left:0.5in">At least
that is my understanding of the situation here.<u></u><u></u></p>
<p class="MsoNormal"><u></u> <u></u></p>
<p class="MsoNormal">Maybe it makes sense to split off
“FoldConstrainedFP” into a separate method, just to
avoid confusion. It would do the same thing
SimplifyCall currently does for constrained FP, just
without tying it to any assumption about the way
SimplifyInstruction works in general.</p>
</div>
</div>
</div>
</div>
</div>
</blockquote>
+1 to this.<br>
<p>Philip<br>
</p>
</div>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div></div>