[PATCH] D73311: [Attributor] Use assumed information to determine side-effects

[Stefan Stipanovic via Phabricator via llvm-commits]

sstefan1 added a comment.

Sorry for the delay.



================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2653
+  /// Determine if \p I is assumed to be side-effect free.
+  bool isAssumedSideEffectFree(Attributor &A, Instruction *I) {
+    if (!I || wouldInstructionBeTriviallyDead(I))
----------------
Few question: 
 - should `isSafeToSpeculativelyExecute` also be checked here?
 - If we had `AASpeculatable` this could be replaced with that, right? Or is it better to use this for speculatable deduction?

While I am at it, speculatable could also be used for `AAUndefinedBehaviour`, rigtht?

Btw, I'll probably start working on speculatable this week.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2704
     if (auto *I = dyn_cast<Instruction>(&V))
-      if (wouldInstructionBeTriviallyDead(I)) {
+      if (isAssumedSideEffectFree(A, I) && !isa<InvokeInst>(I)) {
         A.deleteAfterManifest(*I);
----------------
Shouldn't `I` always be `AssumedSideEffectFree` at this point?


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2807
+
+    // We track this separatly as a secondary state.
+    IsAssumedSideEffectFree = isAssumedSideEffectFree(A, getCtxI());
----------------
typo: separately


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73311