<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><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" 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 Thu, May 5, 2022 at 11:39 PM 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><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>