[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:58:32 PDT 2022
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?
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/1887c30e/attachment.html>
More information about the llvm-commits
mailing list