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

[Nikita Popov via Phabricator via llvm-commits]

nikic 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.
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118426