[PATCH] D21167: [LICM] Make isGuaranteedToExecute more accurate.

[Sanjoy Das via llvm-commits]

sanjoy requested changes to this revision.
sanjoy added a comment.
This revision now requires changes to proceed.

I have some minor nits inline.  However, I think before adding Yet Another Special Case(TM) for the "readonly implies termination" rule we should at least try to scope out how much work it will be to do this right and introduce a "halting" or "always_returns" attribute, and infer them for trivially halting functions.


================
Comment at: lib/Analysis/ValueTracking.cpp:3448
@@ +3447,3 @@
+  // A memory operation returns normally if it isn't volatile. A volatile
+  // operation is allowed to trap.
+  //
----------------
I don't think we model trapping memory operations in the IR at all (volatile or not).

================
Comment at: lib/Analysis/ValueTracking.cpp:3481
@@ +3480,3 @@
+    // global is guaranteed to return.
+    return CS.onlyReadsMemory() || CS.onlyAccessesArgMemory();
+  }
----------------
I'm not sure `onlyAccessesArgMemory` is correct.  What about:

```
void f(volatile int* ptr) {
  while (true) *ptr = 1;
}
```


================
Comment at: lib/IR/Instruction.cpp:15
@@ -14,2 +14,3 @@
 #include "llvm/IR/Instruction.h"
+#include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/CallSite.h"
----------------
Why do you need this?

================
Comment at: lib/Transforms/Scalar/LICM.cpp:401
@@ -400,3 +400,3 @@
        (BB != BBE) && !SafetyInfo->MayThrow; ++BB)
     for (BasicBlock::iterator I = (*BB)->begin(), E = (*BB)->end();
          (I != E) && !SafetyInfo->MayThrow; ++I)
----------------
As a later change, can I ask you to please change these loops to use `all_of` or `any_of`?

================
Comment at: lib/Transforms/Scalar/LICM.cpp:776
@@ +775,3 @@
+  // FIXME: In general, we have to prove that the loop isn't an infinite loop.
+  // See http::llvm.org/PR24078 .  (The "ExitBlocks.empty()" check above is
+  // just a special case of this.)
----------------
Nit: `http://` or just `PR...`.


http://reviews.llvm.org/D21167