[llvm] 83914ee - [InstCombine] Remove side effect of replaced constrained intrinsics
Philip Reames via llvm-commits
llvm-commits at lists.llvm.org
Thu May 5 12:00:02 PDT 2022
On 5/5/22 11:58, Philip Reames wrote:
>
>
> On 5/5/22 11:17, Serge Pavlov wrote:
>> OK, I reverted the change.
> Thank you.
>>
>> 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"?
>
> Ok, I think I talked myself into a corner here and need to back up a
> bit. The "weaker side effecting call" point was wrong. I was
> thinking about simplifyLibCall (which can replace one call with a new
> one), not SimplifyCall which is only allowed to replace a call with an
> existing IR Value.
>
> Given hat, let me stop and explicitly apologize. I explained this
> badly and got myself confused in the process. While I do think it's
> important to promptly revert changes with potential correctness bugs
> noted, I can understand your hesitation given my comments weren't
> making sense. Sorry.
>
>
> I still think this patch needs some adjustment before relanding, but
> the reasoning is a bit different (and not correctness related).
>
> The structure given in the patch is not idiomatic. The idiomatic
> piece would be:
>
> if (sa<ConstrainedFPIntrinsic>(CI)) {
> if (Value *V = SimplifyCall(&CI, SQ.getWithInstruction(&CI))) {
> return IC.replaceAllUsesOf(CI, V);
> }
> }
>
> The key piece here is that using the result of SimplifyX to remove X
> without doing a replacement is very non-idiomatic. It causes readers
> - like say me - to go "why?". Unless I'm, still, missing something,
> the idiomatic usage should cover your case right?
>
This probably isn't sufficient for reasons nikic pointed out. See my
upcoming response to him.
>
>
> Thinking about this deeper though, I somewhat think you're solving
> this problem in the wrong place. Independently of whether we can
> simplify the call, being able to figure out whether the semantics need
> to be "strict" would seem like an interesting transform on it's own.
> Combined with this existing code from wouldInstructionBeTriviallyDead:
>
> if (auto *FPI = dyn_cast<ConstrainedFPIntrinsic>(I)) {
> Optional<fp::ExceptionBehavior> ExBehavior =
> FPI->getExceptionBehavior();
> return ExBehavior.getValue() != fp::ebStrict;
> }
>
> It seems like you'd end up with something strictly more powerful than
> this patch.
>
>
>> 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/20220505/f85a4478/attachment.html>
More information about the llvm-commits
mailing list