[llvm] 83914ee - [InstCombine] Remove side effect of replaced constrained intrinsics
Serge Pavlov via llvm-commits
llvm-commits at lists.llvm.org
Thu May 5 11:17:36 PDT 2022
OK, I reverted the change.
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.
>
Could you please elaborate on this case? Side effect of a constrained
intrinsic is only the interaction with floating point environment:
dependence on control modes, including rounding direction, and change of
floating point exception flags. What do you mean by "weaker side effecting
call"?
Thanks,
--Serge
On Fri, May 6, 2022 at 1:01 AM Philip Reames <listmail at philipreames.com>
wrote:
> 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/20220506/13ca4372/attachment.html>
More information about the llvm-commits
mailing list