[llvm] 83914ee - [InstCombine] Remove side effect of replaced constrained intrinsics
Philip Reames via llvm-commits
llvm-commits at lists.llvm.org
Thu May 5 11:01:18 PDT 2022
Serge,
Please revert this change. If you do not, I will.
On 5/5/22 10:25, Serge Pavlov wrote:
>
> 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.
>
> This path is executed for calls that have no uses. In this case call
> to replaceAllUsesWith does not make sense.
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.
>
> The resulting transformation in this case may be considered as merging
> of two operations:
> 1. SimplifyCall returns ConstantFloat(2.0) and the call is replaced by
> this expression.
> 2. As this ConstantFloat(2.0) is not used, it is removed.
> Both operations are valid and occur in InstCombiner. Their combination
> must be valid also. It is just what the discussed code does.
> Thanks,
> --Serge
>
>
> On Thu, May 5, 2022 at 11:39 PM Philip Reames
> <listmail at philipreames.com> wrote:
>
>
> 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/774266d3/attachment.html>
More information about the llvm-commits
mailing list