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

[Philip Reames via llvm-commits]

On 5/5/22 11:54, Eli Friedman wrote:
>
> *From:* llvm-commits <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>
> *Cc:* Serge Pavlov <llvmlistbot at llvm.org>; llvm-commits 
> <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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20220505/81a6c73f/attachment.html>