[PATCH] D118426: [InstCombine] Remove side effect of replaced constrained intrinsics

[Serge Pavlov via Phabricator via llvm-commits]

sepavloff added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:4134
+            // was found for it, assume that side effect is actually absent and
+            // the instruction can be removed.
+            isa<ConstrainedFPIntrinsic>(I)) {
----------------
efriedma wrote:
> kpn wrote:
> > efriedma wrote:
> > > It's not clear to me this is the only reason a constrained FP intrinsic can be returned.  And even if it were, it's a fragile invariant.
> > > 
> > > Would it make sense to just have isInstructionTriviallyDead() do the right thing here? We already do something similar with isMathLibCallNoop().
> > With "strict" exception behavior we are not allowed to eliminate any run-time traps. I still do not know how isInstructionTriviallyDead() can trivially know that an instruction will never trap and thus can know that removing the instruction is trivially safe.
> It can prove it exactly the same way this patch is trying to do: by constant-folding the operation.
> Would it make sense to just have isInstructionTriviallyDead() do the right thing here? We already do something similar with isMathLibCallNoop().

> It can prove it exactly the same way this patch is trying to do: by constant-folding the operation.

It could but it seems to be impractical. `isInstructionTriviallyDead` is called very often, so it must work enough fast. Making constant evaluations in every invocation of it would substantially increase compile time. The evaluation is made using APFloat, so even for simple operations like `llvm.experimental.constrained.fadd` it involves enough heavy computations. And for functions like `llvm.experimental.constrained.fptosi` or `llvm.experimental.constrained.cos` execution time would be definitely large.

Of course, most users wouldn't suffer from this, because the do not use 'strict' exception behavior. However in strict mode compile time may be unexplainably large and it eventually be noticed and considered as a bug.

Also `isInstructionTriviallyDead` is already overloaded with various checks and attempt adding constant evaluation to it may be considered as undesirable.

On the other hand, the assumption described in this comment is actually obvious and natural. If the code that simplifies IR constructs decides that this constrained intrinsic call may be replaced by some other constructs, we may ignore the side effect of this call. In other words the simplifying code is responsible for taking into account the side effect of the intrinsic.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118426/new/

https://reviews.llvm.org/D118426