[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:07:09 PDT 2022
On 5/5/22 11:44, Nikita Popov wrote:
> On Thu, May 5, 2022 at 6:17 PM Philip Reames via llvm-commits
> <llvm-commits at lists.llvm.org> 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).
>
> Hey Philip,
>
> This is not quite how InstSimplify works: It returns a refinement of
> the instruction return value. The simplification result has no
> relation to modelling of side effects.
Agreed. As noted in the last round of response, I'd gotten myself
confused here. As reading your message reminded me, I was apparently
still confused re: the actual dropping of the intrinsic and had to
correct myself further, but hey, I appear to be at least converging
towards correct. :)
>
> For example, let's say you have "%x = call i32 @foo(i32 returned %y)"
> then InstSimplify could simplify %x to %y, because the return value of
> the call is known, regardless of any side-effects it may have.
> Replacing uses of %x with %y is legal, but removing the call is not.
> (InstSimplify doesn't actually perform this fold because of reasons
> related to CGSSC passes, but it could perform it, and serves as a
> simple illustration.)
>
> Of course, this also means that generally you can't say that "because
> SimplifyCall returned something, you can drop the call" -- that would
> be obviously illegal for the aforementioned fold.
>
> What this patch is doing is to say that for constrained FP intrinsics
> in particular, SimplifyCall gains an additional contract that
> InstSimplify does not generally have: It will only return a value if
> the constrained FP intrinsic is side-effect free.
Yeah, this point makes me really really uncomfortable. You're basically
saying that we're adding an extra clause to InstSimplify's contract
specifically for constrained FP. I don't think we should allow that.
The amount of increased coupling this creates, and room for future bugs
seems to have a high barrier. I'd want to be fairly sure we'd exhausted
other options.
In the extreme, copying the code (which appears to be less than 20
lines) would seem less objectionable.
>
> The reason for this is pragmatic, as determining whether a constrained
> FP call is side-effect free is essentially the same as determining
> whether it can be folded. As such, this patch tries to piggy-back on
> the existing simplification logic to determine whether the call can
> also be removed. This somewhat dirties the API contract, but avoids
> the need to reimplement folding logic in
> wouldInstructionBeTriviallyDead().
If it's just a code reuse point, having an implementation function which
is used by both consumers would seem reasonable. It's specifically the
complication of the instsimplify *interface* which I'm objecting to here.
>
> At least that is my understanding of the situation here.
>
> Regards,
> Nikita
>
>>
>> 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
>>
> _______________________________________________
> 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/99b5e53d/attachment.html>
More information about the llvm-commits
mailing list