[llvm] 83914ee - [InstCombine] Remove side effect of replaced constrained intrinsics
Philip Reames via llvm-commits
llvm-commits at lists.llvm.org
Thu May 5 09:39:36 PDT 2022
On 5/5/22 09:32, Serge Pavlov wrote:
>
> My point is that SimplifyCall is allowed to simplify one side
> effecting call to another side effecting call.
>
>
> 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:
>
> define float @f_unused_precise() #0 {
> %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
> }
>
> 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.
This is the perfect illustration of my point. Consider what happens
when %result is used.
SimplifyCall returns ConstantFloat(2.0) in your example. Dropping the
call *without* doing replaceAllUsesWith is a bug.
Please revert this change. If you want, we can continue discussing on
the review afterwards, but please revert this change now.
>
> Thanks,
> --Serge
>
>
> On Thu, May 5, 2022 at 11:17 PM Philip Reames
> <listmail at philipreames.com> 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).
>
>>
>> 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
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20220505/809e2e7f/attachment.html>
More information about the llvm-commits
mailing list