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

[Sanjoy Das via llvm-commits]

sanjoy accepted this revision.
sanjoy added a subscriber: broune.
sanjoy added a comment.
This revision is now accepted and ready to land.

I had intended to add this myself in http://reviews.llvm.org/D19210 (so I don't have fundamental issues with the change) but decided against it when @broune pointed out that `while(1);` is well defined C11.  But given that http://reviews.llvm.org/D19210 was also LGTM'ed (I was misremembering this earlier), I think this can go in.

> I think there's still some sort of pass manager problem that prevents accessing SCEV from functionattrs?


It is even worse, I think, since not all infinite control flow will show up as `llvm::Loop` instances, so even if you could use SCEV, that won't be enough -- we'd have to conservative in the face of any cyclic control flow we couldn't fully understand.


================
Comment at: lib/Analysis/ValueTracking.cpp:3481
@@ +3480,3 @@
+    // global is guaranteed to return.
+    return CS.onlyReadsMemory() || CS.onlyAccessesArgMemory();
+  }
----------------
eli.friedman wrote:
> 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.
SGTM


http://reviews.llvm.org/D21167