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

[Eli Friedman via llvm-commits]

eli.friedman added a comment.

I don't think anything has changed for "halting" since the last time it was discussed, about a year ago.  I mean, we probably get a lot of the benefit by just marking all the LLVM intrinsics and libcalls halting (which is straightfoward, but tedious to go through and find the exceptions).  We can add some additional markings in clang for stuff marked pure.  Beyond that, for a function definition, the rules are that it doesn't unwind, doesn't call any non-halting function, and doesn't have any infinite loops.  Finding calls to non-halting functions is easy.  Not sure how we solve whether whether a function has an infinite loop; I think that was still up in the air the last time it was discussed.  I think there's still some sort of pass manager problem that prevents accessing SCEV from functionattrs?  Also, some people would prefer to assume side-effect-free loops terminate in C/C++, and it isn't clear exactly what accommodations we have to make for that.

I'm not actually 100% sure about the "doesn't unwind" bit; it might make sense to compute a "doesn't call longjmp" bit separately.


================
Comment at: lib/Analysis/ValueTracking.cpp:3481
@@ +3480,3 @@
+    // global is guaranteed to return.
+    return CS.onlyReadsMemory() || CS.onlyAccessesArgMemory();
+  }
----------------
sanjoy wrote:
> I'm not sure `onlyAccessesArgMemory` is correct.  What about:
> 
> ```
> void f(volatile int* ptr) {
>   while (true) *ptr = 1;
> }
> ```
> 
I don't think volatile stores are allowed in "argmemonly" methods.  It would be like allowing volatile loads in a "readonly" method.  If we say argmemonly functions are allowed to use volatile stores, from an optimization perspective the attribute is exactly equivalent to inaccessiblemem_or_argmemonly.

================
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"
----------------
sanjoy wrote:
> Why do you need this?
Oops, leftover from an older version.


http://reviews.llvm.org/D21167