[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:1057
+        CI.replaceAllUsesWith(V);
+      return ConstrainedIntr;
+    }
----------------
nikic wrote:
> You can't use RAUW in InstCombine. This should be `return replaceInstUsesWith(II, V)`.
> 
> ...actually, as we already called SimplifyCall above, this situation can only happen if you have a constrained intrinsic with use_empty(). I'd suggest to check for use_empty() + simplify succeeds and directly erase the call in that case. I think that will be more obvious that adding a readnone attribute to get it erased in a separate iteration of InstCombine.
You are right. As now all things happen in the same pass, tweaking attributes is not needed. Instead constrained intrinsics can be handled specially in the InstCombine code that removes unused instructions.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:3376
+    // existed, SimplifyInstruction would return null value for this
+    // constrained intrinsic.
+    if (Res != &CI)
----------------
nikic wrote:
> Can you please add a comment to InstructionSimplify that documents this additional requirement? That is, that constrained intrinsics are only allowed to be folded if they do not side effect (which is different from the usual InstSimplify rules).
Added relevant documentation to the declaration of `SimplifyCall`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118426