[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;
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118426