[PATCH] D28147: [LICM] Allow promotion of some stores that are not guaranteed to execute

Philip Reames via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Dec 29 18:51:44 PST 2016


reames added a comment.

In https://reviews.llvm.org/D28147#632595, @mkuper wrote:

> So, the patch is now fairly small, but I'm having trouble with the testcases.
>  In particular, I think what we have now is unsound, because isGuaranteedToTransferExecutionToSuccessor() returns true for argmemonly calls, which means that things that should only get promoted with my patch get promoted without.


We have a prevaling assumption that to throw an exception, you must write to some global (but unknown) location to do so.  We effectively assume that any readonly call is also nounwind.  As you point out, we also assume that argmemonly must be readonly with respect to that unknown magic location.  That doesn't seem sound as the location written to might be reachable through an argument of the function.  So, yes, I think this is wrong.

> I'll get back to this on Tuesday. For now, I'll upload the code change, without fixing the tests, in case you want to take a look at the semantic change.

Thanks.  I took a look and have only minor comments on what's left.  Once we have the above issue fixed, this will be quick.



================
Comment at: lib/Transforms/Scalar/LICM.cpp:928
+  // the store, so inserting a store into the exit block is safe. Note that this
+  // is different from the store being guaranteed to execute. For instance,
+  // if an exception is thrown inside the loop, the original store is not
----------------
if an exception is thrown *on the first iteration*.  


================
Comment at: lib/Transforms/Scalar/LICM.cpp:1012
 
+        // If a store dominates all exit blocks, it is safe to sink.
+        if (!SafeToInsertStore)
----------------
Expand this comment a bit.  possibly: because it must have executed in the original program before reaching one of the inert points.  


https://reviews.llvm.org/D28147





More information about the llvm-commits mailing list