[llvm] 83914ee - [InstCombine] Remove side effect of replaced constrained intrinsics
Nikita Popov via llvm-commits
llvm-commits at lists.llvm.org
Thu May 5 11:44:00 PDT 2022
On Thu, May 5, 2022 at 6:17 PM Philip Reames via llvm-commits <
llvm-commits at lists.llvm.org> wrote:
>
> On 5/5/22 09:14, Serge Pavlov wrote:
>
> This looks incorrect to me. SimplifyCall returns a Value* for a
>> simplified expression and you're interpreting that as a boolean meaning
>> "delete call".
>>
>
> 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.
>
> 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).
>
Hey Philip,
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.
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.)
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.
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.
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().
At least that is my understanding of the situation here.
Regards,
Nikita
> I would expect this to be in isInstructionTriviallyDead anyways.
>>
>
> 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.
>
> Probably it is not a perfect solution, but it seems that we have no other
> solution at the moment. This problem was discussed in
> https://reviews.llvm.org/D118426 and in some previous patch reviews. If
> you have any ideas you could continue that discussion.
>
> Thanks,
> --Serge
>
>
> On Thu, May 5, 2022 at 10:00 PM Philip Reames <listmail at philipreames.com>
> wrote:
>
>>
>> On 5/4/22 22:03, Serge Pavlov via llvm-commits wrote:
>> > Author: Serge Pavlov
>> > Date: 2022-05-05T12:02:42+07:00
>> > New Revision: 83914ee96fc2d828e1cfb8913f5d156d39150e2c
>> >
>> > URL:
>> https://github.com/llvm/llvm-project/commit/83914ee96fc2d828e1cfb8913f5d156d39150e2c
>> > DIFF:
>> https://github.com/llvm/llvm-project/commit/83914ee96fc2d828e1cfb8913f5d156d39150e2c.diff
>> >
>> > LOG: [InstCombine] Remove side effect of replaced constrained intrinsics
>> >
>> > If a constrained intrinsic call was replaced by some value, it was not
>> > removed in some cases. The dangling instruction resulted in useless
>> > instructions executed in runtime. It happened because constrained
>> > intrinsics usually have side effect, it is used to model the interaction
>> > with floating-point environment. In some cases it is correct behavior
>> > but often the side effect is actually absent or can be ignored.
>> >
>> > This change adds specific treatment of constrained intrinsics so that
>> > their side effect can be removed if it actually absents.
>> >
>> > Differential Revision: https://reviews.llvm.org/D118426
>> >
>> > Added:
>> > llvm/test/Transforms/InstCombine/constrained.ll
>> >
>> > Modified:
>> > llvm/include/llvm/Analysis/InstructionSimplify.h
>> > llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
>> >
>> > Removed:
>> >
>> >
>> >
>> >
>> ################################################################################
>> > diff --git a/llvm/include/llvm/Analysis/InstructionSimplify.h
>> b/llvm/include/llvm/Analysis/InstructionSimplify.h
>> > index 8f6ed3a6a1928..612a73551f720 100644
>> > --- a/llvm/include/llvm/Analysis/InstructionSimplify.h
>> > +++ b/llvm/include/llvm/Analysis/InstructionSimplify.h
>> > @@ -299,6 +299,10 @@ Value *SimplifyBinOp(unsigned Opcode, Value *LHS,
>> Value *RHS, FastMathFlags FMF,
>> > const SimplifyQuery &Q);
>> >
>> > /// Given a callsite, fold the result or return null.
>> > +///
>> > +/// \note A call with declared side effect may be simplified into a
>> value
>> > +/// without such. It happens if simplification code deduces that side
>> effect
>> > +/// is actually absent.
>> > Value *SimplifyCall(CallBase *Call, const SimplifyQuery &Q);
>> >
>> > /// Given an operand for a Freeze, see if we can fold the result.
>> >
>> > diff --git a/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
>> b/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
>> > index c96919caee2b1..6b2ab24b4b746 100644
>> > --- a/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
>> > +++ b/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
>> > @@ -1237,6 +1237,16 @@ Instruction
>> *InstCombinerImpl::visitCallInst(CallInst &CI) {
>> > return NewCall;
>> > }
>> >
>> > + // Unused constrained FP intrinsic calls may have declared side
>> effect, which
>> > + // actually absent. If SimplifyCall returns a replacement for such
>> call,
>> > + // assume side effect is absent and the call may be removed.
>> > + if (CI.use_empty() && isa<ConstrainedFPIntrinsic>(CI)) {
>> > + if (SimplifyCall(&CI, SQ.getWithInstruction(&CI))) {
>> > + eraseInstFromFunction(CI);
>> > + return nullptr;
>> > + }
>> > + }
>> > +
>>
>> This looks incorrect to me. SimplifyCall returns a Value* for a
>> simplified expression and you're interpreting that as a boolean meaning
>> "delete call".
>>
>> I would expect this to be in isInstructionTriviallyDead anyways.
>>
>> Please revert unless I'm misreading.
>>
>> > Intrinsic::ID IID = II->getIntrinsicID();
>> > switch (IID) {
>> > case Intrinsic::objectsize:
>> >
>> > diff --git a/llvm/test/Transforms/InstCombine/constrained.ll
>> b/llvm/test/Transforms/InstCombine/constrained.ll
>> > new file mode 100644
>> > index 0000000000000..b5ef71e6edfba
>> > --- /dev/null
>> > +++ b/llvm/test/Transforms/InstCombine/constrained.ll
>> > @@ -0,0 +1,125 @@
>> > +; NOTE: Assertions have been autogenerated by
>> utils/update_test_checks.py
>> > +; RUN: opt -S -instcombine %s | FileCheck %s
>> > +
>> > +; Treatment of operation with unused result.
>> > +
>> > +; If operation does not raise exceptions, it may be removed even in
>> strict mode.
>> > +define float @f_unused_precise() #0 {
>> > +; CHECK-LABEL: @f_unused_precise(
>> > +; CHECK-NEXT: entry:
>> > +; CHECK-NEXT: ret float 1.000000e+00
>> > +;
>> > +entry:
>> > + %result = call float @llvm.experimental.constrained.fadd.f32(float
>> 1.0, float 1.0, metadata !"round.upward", metadata !"fpexcept.strict") #0
>> > + ret float 1.0
>> > +}
>> > +
>> > +; If operation raises exceptions, it cannot be removed in strict mode.
>> > +define float @f_unused_strict() #0 {
>> > +; CHECK-LABEL: @f_unused_strict(
>> > +; CHECK-NEXT: entry:
>> > +; 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]+]]
>> > +; CHECK-NEXT: ret float 1.000000e+00
>> > +;
>> > +entry:
>> > + %result = call float @llvm.experimental.constrained.fdiv.f32(float
>> 1.0, float 3.0, metadata !"round.tonearest", metadata !"fpexcept.strict") #0
>> > + ret float 1.0
>> > +}
>> > +
>> > +; If operation raises exceptions, it can be removed in non-strict mode.
>> > +define float @f_unused_ignore() #0 {
>> > +; CHECK-LABEL: @f_unused_ignore(
>> > +; CHECK-NEXT: entry:
>> > +; CHECK-NEXT: ret float 1.000000e+00
>> > +;
>> > +entry:
>> > + %result = call float @llvm.experimental.constrained.fdiv.f32(float
>> 1.0, float 3.0, metadata !"round.towardzero", metadata !"fpexcept.ignore")
>> #0
>> > + ret float 1.0
>> > +}
>> > +
>> > +; If operation raises exceptions, it can be removed in non-strict mode
>> even if rounding mode is dynamic.
>> > +define float @f_unused_dynamic_ignore() #0 {
>> > +; CHECK-LABEL: @f_unused_dynamic_ignore(
>> > +; CHECK-NEXT: entry:
>> > +; CHECK-NEXT: ret float 1.000000e+00
>> > +;
>> > +entry:
>> > + %result = call float @llvm.experimental.constrained.fdiv.f32(float
>> 1.0, float 3.0, metadata !"round.dynamic", metadata !"fpexcept.ignore") #0
>> > + ret float 1.0
>> > +}
>> > +
>> > +; If operation raises exceptions, it can be removed in "maytrap" mode.
>> > +define float @f_unused_maytrap() #0 {
>> > +; CHECK-LABEL: @f_unused_maytrap(
>> > +; CHECK-NEXT: entry:
>> > +; CHECK-NEXT: ret float 1.000000e+00
>> > +;
>> > +entry:
>> > + %result = call float @llvm.experimental.constrained.fdiv.f32(float
>> 1.0, float 3.0, metadata !"round.tonearest", metadata !"fpexcept.maytrap")
>> #0
>> > + ret float 1.0
>> > +}
>> > +
>> > +; Constant evaluation.
>> > +
>> > +; If operation does not raise exceptions, it may be folded even in
>> strict mode.
>> > +define float @f_eval_precise() #0 {
>> > +; CHECK-LABEL: @f_eval_precise(
>> > +; CHECK-NEXT: entry:
>> > +; CHECK-NEXT: ret float 2.000000e+00
>> > +;
>> > +entry:
>> > + %result = call float @llvm.experimental.constrained.fadd.f32(float
>> 1.0, float 1.0, metadata !"round.upward", metadata !"fpexcept.strict") #0
>> > + ret float %result
>> > +}
>> > +
>> > +; If operation raises exceptions, it cannot be folded in strict mode.
>> > +define float @f_eval_strict() #0 {
>> > +; CHECK-LABEL: @f_eval_strict(
>> > +; CHECK-NEXT: entry:
>> > +; 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]]
>> > +; CHECK-NEXT: ret float [[RESULT]]
>> > +;
>> > +entry:
>> > + %result = call float @llvm.experimental.constrained.fdiv.f32(float
>> 1.0, float 3.0, metadata !"round.upward", metadata !"fpexcept.strict") #0
>> > + ret float %result
>> > +}
>> > +
>> > +; If operation raises exceptions, it can be folded in non-strict mode.
>> > +define float @f_eval_ignore() #0 {
>> > +; CHECK-LABEL: @f_eval_ignore(
>> > +; CHECK-NEXT: entry:
>> > +; CHECK-NEXT: ret float 0x3FD5555540000000
>> > +;
>> > +entry:
>> > + %result = call float @llvm.experimental.constrained.fdiv.f32(float
>> 1.0, float 3.0, metadata !"round.downward", metadata !"fpexcept.ignore") #0
>> > + ret float %result
>> > +}
>> > +
>> > +; if result is imprecise, it cannot be folded if rounding mode is
>> dynamic.
>> > +define float @f_eval_dynamic_ignore() #0 {
>> > +; CHECK-LABEL: @f_eval_dynamic_ignore(
>> > +; CHECK-NEXT: entry:
>> > +; 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]]
>> > +; CHECK-NEXT: ret float [[RESULT]]
>> > +;
>> > +entry:
>> > + %result = call float @llvm.experimental.constrained.fdiv.f32(float
>> 1.0, float 3.0, metadata !"round.dynamic", metadata !"fpexcept.ignore") #0
>> > + ret float %result
>> > +}
>> > +
>> > +; If result is imprecise and rounding mode is not dynamic, operation
>> can be folded in "maytrap" mode.
>> > +define float @f_eval_maytrap() #0 {
>> > +; CHECK-LABEL: @f_eval_maytrap(
>> > +; CHECK-NEXT: entry:
>> > +; CHECK-NEXT: ret float 0x3FD5555560000000
>> > +;
>> > +entry:
>> > + %result = call float @llvm.experimental.constrained.fdiv.f32(float
>> 1.0, float 3.0, metadata !"round.tonearest", metadata !"fpexcept.maytrap")
>> #0
>> > + ret float %result
>> > +}
>> > +
>> > +
>> > +declare float @llvm.experimental.constrained.fadd.f32(float, float,
>> metadata, metadata)
>> > +declare float @llvm.experimental.constrained.fdiv.f32(float, float,
>> metadata, metadata)
>> > +
>> > +attributes #0 = { strictfp }
>> >
>> >
>> >
>> > _______________________________________________
>> > llvm-commits mailing list
>> > llvm-commits at lists.llvm.org
>> > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20220505/434f0212/attachment.html>
More information about the llvm-commits
mailing list