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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert marked 2 inline comments as done.
jdoerfert added inline comments.


================
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))
----------------
sstefan1 wrote:
> 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.
> should isSafeToSpeculativelyExecute also be checked here?

I don't think so. We can delete the code if it is side-effect free and has no live users.

> If we had AASpeculatable this could be replaced with that, right? Or is it better to use this for speculatable deduction?

So `AASpeculatable` would derive the `speculatable` attribute, which allows us and other passes to move code out of control dependences, e.g.,
`if (...) do_sth_speculatable();` can become `do_sth_speculatable(); if (...) ;`

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

Let's discuss that on the AAspeculatable patch, I'm not 100% sure right now what you mean.

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

That sounds great!


================
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);
----------------
sstefan1 wrote:
> Shouldn't `I` always be `AssumedSideEffectFree` at this point?
That's a good point. I'll check why this is here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73311