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

[Eli Friedman via llvm-commits]

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<mailto: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.

-Eli
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20220505/87f0583d/attachment.html>