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

[Eli Friedman via Phabricator via llvm-commits]

efriedma 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)) {
----------------
kpn wrote:
> sepavloff wrote:
> > 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.
> Personally, I would like to see isInstructionTriviallyDead() do the right thing. But the current plan to call back in to see if we can constant fold doesn't scale. Look at my changes to InstSimplify to see simplification cases that aren't constant folding.
> 
> > In other words the simplifying code is responsible for taking into account the side effect of the intrinsic.
> 
> Yes. Serge hit the nail on the head.
> 
> If we go the route of having isInstructionTriviallyDead() re-derive the fact that an instruction can be eliminated by re-running code to determine it is safe then we need to also re-determine that the non-constant folding simplifications are safe. We would also need to re-run any other transformation-handling code that comes up in the future. That seems terribly heavyweight, and bad layering as well.
> 
> In an earlier ticket we saw Serge's suggestion of having transformation passes annotate instructions so the existing isInstructionTriviallyDead() implementation will recognize they can be trivially deleted.  This suggestion was rejected for reasons I never saw explained. I'd love to understand the issues with that proposed solution at least.
> 
> That leaves us with options like this ticket, where instructions to be deleted hang around for multiple passes until InstCombine gets run. This solution also has, to a lesser extent I think, some of the scalability issues outlined above on top of being fragile as @efriedma pointed out.
> 
> Does anyone have any other suggestions that would solve the problem without creating new ones? Anyone?
> In an earlier ticket we saw Serge's suggestion of having transformation passes annotate instructions so the existing isInstructionTriviallyDead() implementation will recognize they can be trivially deleted

Which is this?  Don't recall seeing this.

------

I think to avoid the issue with hacking at InstCombinerImpl::run, you can do something like the following in visitCallInst:

```
  if (CI.use_empty() && isa<ConstrainedFPIntrinsic>(CI)) {
    if (SimplifyCall(&CI, SQ.getWithInstruction(&CI))) {
      eraseInstFromFunction(CI);
      return nullptr;
    }
  }
```




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118426