<div dir="ltr"><div dir="ltr">OK, I reverted the change.</div><div dir="ltr"><br></div><div dir="ltr"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">For the constant example, you're correct. If SimplifyCall decided to use e.g. a weaker side effecting call, you still have the same problem.<br clear="all"></blockquote><div><br></div><div>Could you please elaborate on this case? Side effect of a constrained intrinsic is only the interaction with floating point environment: dependence on control modes, including rounding direction, and change of floating point exception flags. What do you mean by "weaker side effecting call"?</div><div><br></div><div><div dir="ltr" class="gmail_signature" data-smartmail="gmail_signature">Thanks,<br>--Serge<br></div></div><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, May 6, 2022 at 1:01 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">
<div>
<p>Serge,</p>
<p>Please revert this change. If you do not, I will. <br>
</p>
<div>On 5/5/22 10:25, Serge Pavlov wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<p>This is the perfect illustration of my point. Consider
what happens when %result is used. <br>
</p>
<p>SimplifyCall returns ConstantFloat(2.0) in your example.
Dropping the call *without* doing replaceAllUsesWith is a
bug. </p>
</blockquote>
<div>This path is executed for calls that have no uses. In this
case call to replaceAllUsesWith does not make sense.</div>
</div>
</blockquote>
<p>For the constant example, you're correct. If SimplifyCall
decided to use e.g. a weaker side effecting call, you still have
the same problem.</p>
<blockquote type="cite">
<div dir="ltr">
<div><br>
</div>
<div>The resulting transformation in this case may be considered
as merging of two operations:</div>
<div>1. SimplifyCall returns ConstantFloat(2.0) and the call is
replaced by this expression.</div>
<div>2. As this ConstantFloat(2.0) is not used, it is removed.</div>
<div>Both operations are valid and occur in InstCombiner. Their
combination must be valid also. It is just what the discussed
code does.</div>
<div> </div>
<div>
<div dir="ltr">Thanks,<br>
--Serge<br>
</div>
</div>
<br>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Thu, May 5, 2022 at 11:39
PM Philip Reames <<a href="mailto:listmail@philipreames.com" target="_blank">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><br>
</p>
<div>On 5/5/22 09:32, Serge Pavlov wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">My point is that
SimplifyCall is allowed to simplify one side effecting
call to another side effecting call.<br clear="all">
</blockquote>
<div><br>
</div>
<div>It also may simplify side effecting call to
something that has no side effect, if the side effect
is actually absent. It takes place for the code: </div>
<div><br>
</div>
<div>define float @f_unused_precise() #0 {<br>
%result = call float
@llvm.experimental.constrained.fadd.f32(float 1.0,
float 1.0, metadata !"round.upward", metadata
!"fpexcept.strict") #0<br>
ret float 1.0<br>
}<br>
</div>
<div><br>
</div>
<div>In it experimental.constrained.fadd has side
effect, because exception behavior is strict, but
SimplifyCall can reveal that actually side effect is
absent and simplifies the call to the constant 2.0. As
the result of the call is not used the net result is
removal of the call.</div>
</div>
</blockquote>
<p>This is the perfect illustration of my point. Consider
what happens when %result is used. <br>
</p>
<p>SimplifyCall returns ConstantFloat(2.0) in your example.
Dropping the call *without* doing replaceAllUsesWith is a
bug. </p>
<p>Please revert this change. If you want, we can continue
discussing on the review afterwards, but please revert
this change now. <br>
</p>
<blockquote type="cite">
<div dir="ltr">
<div><br>
</div>
<div>
<div dir="ltr">Thanks,<br>
--Serge<br>
</div>
</div>
<br>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Thu, May 5, 2022 at
11:17 PM Philip Reames <<a href="mailto:listmail@philipreames.com" target="_blank">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><br>
</p>
<div>On 5/5/22 09:14, Serge Pavlov wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">
<div dir="ltr">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">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".<br>
</blockquote>
<div><br>
</div>
<div>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. <br>
</div>
</div>
</div>
</blockquote>
<p>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). <br>
</p>
<blockquote type="cite">
<div dir="ltr">
<div dir="ltr">
<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">I would
expect this to be in
isInstructionTriviallyDead anyways.<br>
</blockquote>
<div><br>
</div>
<div>It would be nice but the concern is that
simplifying such calls is expensive and
isInstructionTriviallyDead is called often
enough, so negative impact on compile time
is unavoidable. There are many
transformations, not only constant folding
and their number will increase, so putting
all of them into isInstructionTriviallyDead
does not look like a good design.</div>
<div><br>
</div>
<div>Probably it is not a perfect solution,
but it seems that we have no other solution
at the moment. This problem was discussed
in <a href="https://reviews.llvm.org/D118426" rel="noreferrer" target="_blank">https://reviews.llvm.org/D118426</a> and
in some previous patch reviews. If you have
any ideas you could continue that
discussion.</div>
<div> </div>
<div>
<div dir="ltr">Thanks,<br>
--Serge<br>
</div>
</div>
<br>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Thu, May
5, 2022 at 10:00 PM Philip Reames <<a href="mailto:listmail@philipreames.com" target="_blank">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"><br>
On 5/4/22 22:03, Serge Pavlov via
llvm-commits wrote:<br>
> Author: Serge Pavlov<br>
> Date: 2022-05-05T12:02:42+07:00<br>
> New Revision:
83914ee96fc2d828e1cfb8913f5d156d39150e2c<br>
><br>
> URL: <a href="https://github.com/llvm/llvm-project/commit/83914ee96fc2d828e1cfb8913f5d156d39150e2c" rel="noreferrer" target="_blank">https://github.com/llvm/llvm-project/commit/83914ee96fc2d828e1cfb8913f5d156d39150e2c</a><br>
> DIFF: <a href="https://github.com/llvm/llvm-project/commit/83914ee96fc2d828e1cfb8913f5d156d39150e2c.diff" rel="noreferrer" target="_blank">https://github.com/llvm/llvm-project/commit/83914ee96fc2d828e1cfb8913f5d156d39150e2c.diff</a><br>
><br>
> LOG: [InstCombine] Remove side effect
of replaced constrained intrinsics<br>
><br>
> If a constrained intrinsic call was
replaced by some value, it was not<br>
> removed in some cases. The dangling
instruction resulted in useless<br>
> instructions executed in runtime. It
happened because constrained<br>
> intrinsics usually have side effect, it
is used to model the interaction<br>
> with floating-point environment. In
some cases it is correct behavior<br>
> but often the side effect is actually
absent or can be ignored.<br>
><br>
> This change adds specific treatment of
constrained intrinsics so that<br>
> their side effect can be removed if it
actually absents.<br>
><br>
> Differential Revision: <a href="https://reviews.llvm.org/D118426" rel="noreferrer" target="_blank">https://reviews.llvm.org/D118426</a><br>
><br>
> Added:<br>
>
llvm/test/Transforms/InstCombine/constrained.ll<br>
><br>
> Modified:<br>
>
llvm/include/llvm/Analysis/InstructionSimplify.h<br>
>
llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp<br>
><br>
> Removed:<br>
> <br>
><br>
><br>
>
################################################################################<br>
> diff --git
a/llvm/include/llvm/Analysis/InstructionSimplify.h
b/llvm/include/llvm/Analysis/InstructionSimplify.h<br>
> index 8f6ed3a6a1928..612a73551f720
100644<br>
> ---
a/llvm/include/llvm/Analysis/InstructionSimplify.h<br>
> +++
b/llvm/include/llvm/Analysis/InstructionSimplify.h<br>
> @@ -299,6 +299,10 @@ Value
*SimplifyBinOp(unsigned Opcode, Value *LHS,
Value *RHS, FastMathFlags FMF,<br>
> const
SimplifyQuery &Q);<br>
> <br>
> /// Given a callsite, fold the result
or return null.<br>
> +///<br>
> +/// \note A call with declared side
effect may be simplified into a value<br>
> +/// without such. It happens if
simplification code deduces that side effect<br>
> +/// is actually absent.<br>
> Value *SimplifyCall(CallBase *Call,
const SimplifyQuery &Q);<br>
> <br>
> /// Given an operand for a Freeze,
see if we can fold the result.<br>
><br>
> diff --git
a/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
b/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp<br>
> index c96919caee2b1..6b2ab24b4b746
100644<br>
> ---
a/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp<br>
> +++
b/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp<br>
> @@ -1237,6 +1237,16 @@ Instruction
*InstCombinerImpl::visitCallInst(CallInst
&CI) {<br>
> return NewCall;<br>
> }<br>
> <br>
> + // Unused constrained FP intrinsic
calls may have declared side effect, which<br>
> + // actually absent. If SimplifyCall
returns a replacement for such call,<br>
> + // assume side effect is absent and
the call may be removed.<br>
> + if (CI.use_empty() &&
isa<ConstrainedFPIntrinsic>(CI)) {<br>
> + if (SimplifyCall(&CI,
SQ.getWithInstruction(&CI))) {<br>
> + eraseInstFromFunction(CI);<br>
> + return nullptr;<br>
> + }<br>
> + }<br>
> +<br>
<br>
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".<br>
<br>
I would expect this to be in
isInstructionTriviallyDead anyways.<br>
<br>
Please revert unless I'm misreading.<br>
<br>
> Intrinsic::ID IID =
II->getIntrinsicID();<br>
> switch (IID) {<br>
> case Intrinsic::objectsize:<br>
><br>
> diff --git
a/llvm/test/Transforms/InstCombine/constrained.ll
b/llvm/test/Transforms/InstCombine/constrained.ll<br>
> new file mode 100644<br>
> index 0000000000000..b5ef71e6edfba<br>
> --- /dev/null<br>
> +++
b/llvm/test/Transforms/InstCombine/constrained.ll<br>
> @@ -0,0 +1,125 @@<br>
> +; NOTE: Assertions have been
autogenerated by utils/update_test_checks.py<br>
> +; RUN: opt -S -instcombine %s |
FileCheck %s<br>
> +<br>
> +; Treatment of operation with unused
result.<br>
> +<br>
> +; If operation does not raise
exceptions, it may be removed even in strict
mode.<br>
> +define float @f_unused_precise() #0 {<br>
> +; CHECK-LABEL: @f_unused_precise(<br>
> +; CHECK-NEXT: entry:<br>
> +; CHECK-NEXT: ret float
1.000000e+00<br>
> +;<br>
> +entry:<br>
> + %result = call float
@llvm.experimental.constrained.fadd.f32(float
1.0, float 1.0, metadata !"round.upward",
metadata !"fpexcept.strict") #0<br>
> + ret float 1.0<br>
> +}<br>
> +<br>
> +; If operation raises exceptions, it
cannot be removed in strict mode.<br>
> +define float @f_unused_strict() #0 {<br>
> +; CHECK-LABEL: @f_unused_strict(<br>
> +; CHECK-NEXT: entry:<br>
> +; CHECK-NEXT: [[RESULT:%.*]] = call
float
@llvm.experimental.constrained.fdiv.f32(float
1.000000e+00, float 3.000000e+00, metadata
!"round.tonearest", metadata
!"fpexcept.strict") #[[ATTR0:[0-9]+]]<br>
> +; CHECK-NEXT: ret float
1.000000e+00<br>
> +;<br>
> +entry:<br>
> + %result = call float
@llvm.experimental.constrained.fdiv.f32(float
1.0, float 3.0, metadata !"round.tonearest",
metadata !"fpexcept.strict") #0<br>
> + ret float 1.0<br>
> +}<br>
> +<br>
> +; If operation raises exceptions, it
can be removed in non-strict mode.<br>
> +define float @f_unused_ignore() #0 {<br>
> +; CHECK-LABEL: @f_unused_ignore(<br>
> +; CHECK-NEXT: entry:<br>
> +; CHECK-NEXT: ret float
1.000000e+00<br>
> +;<br>
> +entry:<br>
> + %result = call float
@llvm.experimental.constrained.fdiv.f32(float
1.0, float 3.0, metadata
!"round.towardzero", metadata
!"fpexcept.ignore") #0<br>
> + ret float 1.0<br>
> +}<br>
> +<br>
> +; If operation raises exceptions, it
can be removed in non-strict mode even if
rounding mode is dynamic.<br>
> +define float
@f_unused_dynamic_ignore() #0 {<br>
> +; CHECK-LABEL:
@f_unused_dynamic_ignore(<br>
> +; CHECK-NEXT: entry:<br>
> +; CHECK-NEXT: ret float
1.000000e+00<br>
> +;<br>
> +entry:<br>
> + %result = call float
@llvm.experimental.constrained.fdiv.f32(float
1.0, float 3.0, metadata !"round.dynamic",
metadata !"fpexcept.ignore") #0<br>
> + ret float 1.0<br>
> +}<br>
> +<br>
> +; If operation raises exceptions, it
can be removed in "maytrap" mode.<br>
> +define float @f_unused_maytrap() #0 {<br>
> +; CHECK-LABEL: @f_unused_maytrap(<br>
> +; CHECK-NEXT: entry:<br>
> +; CHECK-NEXT: ret float
1.000000e+00<br>
> +;<br>
> +entry:<br>
> + %result = call float
@llvm.experimental.constrained.fdiv.f32(float
1.0, float 3.0, metadata !"round.tonearest",
metadata !"fpexcept.maytrap") #0<br>
> + ret float 1.0<br>
> +}<br>
> +<br>
> +; Constant evaluation.<br>
> +<br>
> +; If operation does not raise
exceptions, it may be folded even in strict
mode.<br>
> +define float @f_eval_precise() #0 {<br>
> +; CHECK-LABEL: @f_eval_precise(<br>
> +; CHECK-NEXT: entry:<br>
> +; CHECK-NEXT: ret float
2.000000e+00<br>
> +;<br>
> +entry:<br>
> + %result = call float
@llvm.experimental.constrained.fadd.f32(float
1.0, float 1.0, metadata !"round.upward",
metadata !"fpexcept.strict") #0<br>
> + ret float %result<br>
> +}<br>
> +<br>
> +; If operation raises exceptions, it
cannot be folded in strict mode.<br>
> +define float @f_eval_strict() #0 {<br>
> +; CHECK-LABEL: @f_eval_strict(<br>
> +; CHECK-NEXT: entry:<br>
> +; CHECK-NEXT: [[RESULT:%.*]] = call
float
@llvm.experimental.constrained.fdiv.f32(float
1.000000e+00, float 3.000000e+00, metadata
!"round.upward", metadata
!"fpexcept.strict") #[[ATTR0]]<br>
> +; CHECK-NEXT: ret float [[RESULT]]<br>
> +;<br>
> +entry:<br>
> + %result = call float
@llvm.experimental.constrained.fdiv.f32(float
1.0, float 3.0, metadata !"round.upward",
metadata !"fpexcept.strict") #0<br>
> + ret float %result<br>
> +}<br>
> +<br>
> +; If operation raises exceptions, it
can be folded in non-strict mode.<br>
> +define float @f_eval_ignore() #0 {<br>
> +; CHECK-LABEL: @f_eval_ignore(<br>
> +; CHECK-NEXT: entry:<br>
> +; CHECK-NEXT: ret float
0x3FD5555540000000<br>
> +;<br>
> +entry:<br>
> + %result = call float
@llvm.experimental.constrained.fdiv.f32(float
1.0, float 3.0, metadata !"round.downward",
metadata !"fpexcept.ignore") #0<br>
> + ret float %result<br>
> +}<br>
> +<br>
> +; if result is imprecise, it cannot be
folded if rounding mode is dynamic.<br>
> +define float @f_eval_dynamic_ignore()
#0 {<br>
> +; CHECK-LABEL: @f_eval_dynamic_ignore(<br>
> +; CHECK-NEXT: entry:<br>
> +; CHECK-NEXT: [[RESULT:%.*]] = call
float
@llvm.experimental.constrained.fdiv.f32(float
1.000000e+00, float 3.000000e+00, metadata
!"round.dynamic", metadata
!"fpexcept.ignore") #[[ATTR0]]<br>
> +; CHECK-NEXT: ret float [[RESULT]]<br>
> +;<br>
> +entry:<br>
> + %result = call float
@llvm.experimental.constrained.fdiv.f32(float
1.0, float 3.0, metadata !"round.dynamic",
metadata !"fpexcept.ignore") #0<br>
> + ret float %result<br>
> +}<br>
> +<br>
> +; If result is imprecise and rounding
mode is not dynamic, operation can be folded
in "maytrap" mode.<br>
> +define float @f_eval_maytrap() #0 {<br>
> +; CHECK-LABEL: @f_eval_maytrap(<br>
> +; CHECK-NEXT: entry:<br>
> +; CHECK-NEXT: ret float
0x3FD5555560000000<br>
> +;<br>
> +entry:<br>
> + %result = call float
@llvm.experimental.constrained.fdiv.f32(float
1.0, float 3.0, metadata !"round.tonearest",
metadata !"fpexcept.maytrap") #0<br>
> + ret float %result<br>
> +}<br>
> +<br>
> +<br>
> +declare float
@llvm.experimental.constrained.fadd.f32(float,
float, metadata, metadata)<br>
> +declare float
@llvm.experimental.constrained.fdiv.f32(float,
float, metadata, metadata)<br>
> +<br>
> +attributes #0 = { strictfp }<br>
><br>
><br>
> <br>
>
_______________________________________________<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>
</blockquote>
</div>
</blockquote>
</div>
</blockquote>
</div>
</blockquote>
</div>
</blockquote>
</div>
</blockquote></div></div>