[llvm] 83914ee - [InstCombine] Remove side effect of replaced constrained intrinsics

[Serge Pavlov via llvm-commits]

Thanks to all for feedback!

I updated the revision https://reviews.llvm.org/D118426 accordingly.

On Fri, May 6, 2022 at 1:58 AM Philip Reames <listmail at philipreames.com>
wrote:

> 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);
>   }
> }
>
Using replaceAllUsesOf is not suitable here because it won't work with
instructions without uses:
https://github.com/llvm/llvm-project/blob/7dc3c6190ec7191dd104fa5158fe0ee32e9b0c49/llvm/lib/Transforms/InstCombine/InstCombineInternal.h#L407

Function InstCombiner::eraseInstFromFunction is widely used in IC,
VisitCall already has several calls of it, so it should not be a problem to
use it here.

Thanks,
--Serge


On Fri, May 6, 2022 at 2:11 AM Philip Reames via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

>
> On 5/5/22 11:54, Eli Friedman wrote:
>
>
>
> *From:* llvm-commits <llvm-commits-bounces at lists.llvm.org>
> <llvm-commits-bounces at lists.llvm.org> *On Behalf Of *Nikita Popov via
> llvm-commits
> *Sent:* Thursday, May 5, 2022 11:44 AM
> *To:* Philip Reames <listmail at philipreames.com>
> <listmail at philipreames.com>
> *Cc:* Serge Pavlov <llvmlistbot at llvm.org> <llvmlistbot at llvm.org>;
> llvm-commits <llvm-commits at lists.llvm.org> <llvm-commits at lists.llvm.org>
> *Subject:* Re: [llvm] 83914ee - [InstCombine] Remove side effect of
> replaced constrained intrinsics
>
>
>
> *WARNING:* This email originated from outside of Qualcomm. Please be wary
> of any links or attachments, and do not enable macros.
>
> 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.
>
>
>
> 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.
>
>
>
> 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().
>
>
>
> At least that is my understanding of the situation here.
>
>
>
> Maybe it makes sense to split off “FoldConstrainedFP” into a separate
> method, just to avoid confusion.  It would do the same thing SimplifyCall
> currently does for constrained FP, just without tying it to any assumption
> about the way SimplifyInstruction works in general.
>
> +1 to this.
>
> Philip
> _______________________________________________
> 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/f0c5e514/attachment-0001.html>