<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<p><br>
</p>
<div class="moz-cite-prefix">On 5/5/22 11:44, Nikita Popov wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CAF+90c-j=vkoJpwG3J_uSz=ckwkL=BrnpLkMqi7FPOgOUmBwMA@mail.gmail.com">
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
<div dir="ltr">
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Thu, May 5, 2022 at 6:17
PM Philip Reames via llvm-commits <<a
href="mailto:llvm-commits@lists.llvm.org"
moz-do-not-send="true" class="moz-txt-link-freetext">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 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>
</div>
</blockquote>
<div>Hey Philip,</div>
<div><br>
</div>
<div>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.<br>
</div>
</div>
</div>
</blockquote>
Agreed. As noted in the last round of response, I'd gotten myself
confused here. As reading your message reminded me, I was
apparently still confused re: the actual dropping of the intrinsic
and had to correct myself further, but hey, I appear to be at least
converging towards correct. :)<br>
<blockquote type="cite"
cite="mid:CAF+90c-j=vkoJpwG3J_uSz=ckwkL=BrnpLkMqi7FPOgOUmBwMA@mail.gmail.com">
<div dir="ltr">
<div class="gmail_quote">
<div><br>
</div>
<div>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.)</div>
<div><br>
</div>
<div>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.</div>
<div><br>
</div>
<div>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.</div>
</div>
</div>
</blockquote>
<p>Yeah, this point makes me really really uncomfortable. You're
basically saying that we're adding an extra clause to
InstSimplify's contract specifically for constrained FP. I don't
think we should allow that. The amount of increased coupling this
creates, and room for future bugs seems to have a high barrier.
I'd want to be fairly sure we'd exhausted other options.</p>
<p>In the extreme, copying the code (which appears to be less than
20 lines) would seem less objectionable. <br>
</p>
<blockquote type="cite"
cite="mid:CAF+90c-j=vkoJpwG3J_uSz=ckwkL=BrnpLkMqi7FPOgOUmBwMA@mail.gmail.com">
<div dir="ltr">
<div class="gmail_quote">
<div><br>
</div>
<div>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().</div>
</div>
</div>
</blockquote>
If it's just a code reuse point, having an implementation function
which is used by both consumers would seem reasonable. It's
specifically the complication of the instsimplify *interface* which
I'm objecting to here. <br>
<blockquote type="cite"
cite="mid:CAF+90c-j=vkoJpwG3J_uSz=ckwkL=BrnpLkMqi7FPOgOUmBwMA@mail.gmail.com">
<div dir="ltr">
<div class="gmail_quote">
<div><br>
</div>
<div>At least that is my understanding of the situation here.<br>
</div>
<div><br>
</div>
<div>Regards,<br>
</div>
<div>Nikita<br>
</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">
<div>
<p> </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"
moz-do-not-send="true"
class="moz-txt-link-freetext">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" moz-do-not-send="true"
class="moz-txt-link-freetext">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"
moz-do-not-send="true"
class="moz-txt-link-freetext">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"
moz-do-not-send="true"
class="moz-txt-link-freetext">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"
moz-do-not-send="true"
class="moz-txt-link-freetext">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" moz-do-not-send="true"
class="moz-txt-link-freetext">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"
class="moz-txt-link-freetext">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote>
</div>
</div>
</blockquote>
</div>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank"
moz-do-not-send="true" class="moz-txt-link-freetext">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"
class="moz-txt-link-freetext">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote>
</div>
</div>
</blockquote>
</body>
</html>