[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/InstCombineCalls.cpp:3373
+    if (CI.getNumUses() == 0)
+      CI.addFnAttr(Attribute::ReadNone);
+    return Res;
----------------
nikic wrote:
> sepavloff wrote:
> > efriedma wrote:
> > > I'm not sure I understand the relevant invariant here.  You're using SimplifyInstruction to assert that the simplified instruction has no side-effects?  That isn't documented anywhere, but I guess it makes sense?  Please confirm and add API documentation explaining this.
> > > 
> > > If it is safe, we should simplify all calls, not just for ConstrainedFPIntrinsic calls.
> > > 
> > > Also, you can just erase a call using eraseInstFromFunction; you don't need to mark it readnone and wait for another iteration of instcombine.
> > I admit the code is not obvious enough.
> > 
> > The key point here is that `SimplifyInstruction` does not return a replacement if the constrained intrinsic has side effect. So if control passed inside the `if` statement, we know that no actual side effect is associated with the intrinsic.
> > 
> > The next check `if (CI.getNumUses() == 0)` handles dangling instructions. They could not be removed due to the side effect but actually they have no such because `SimplifyInstruction` provided a replacement.
> > 
> > > Also, you can just erase a call using eraseInstFromFunction;
> > 
> > InstCombiner will do it in the same iteration, if `isInstructionTriviallyDead` returns `true` for this instruction.
> > If it is safe, we should simplify all calls, not just for ConstrainedFPIntrinsic calls.
> 
> This is definitely not safe in general -- obvious example being math libcalls that fold but are not dead due to errno. This needs to be constrained FP intrinsic specific contract.
If `SimplifyInstruction` returned a replacement even if side effect presents and the exception behavior is strict, the original instruction would not be deleted, because it is unknown if the side effect actually exists. `SimplifyInstruction` is an analysis, it cannot remove bogus side effect. To find out if the side effect actually absents the logic of `SimplifyInstruction` must be replayed, which is undesirable as it would increase compile time.

If `SimplifyInstruction` returns replacement only when it can replace the original expression without change of semantic, the useless code can be removed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118426