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

[Michael Kuperstein via Phabricator via llvm-commits]

mkuper added inline comments.


================
Comment at: test/Transforms/LICM/scalar_promote.ll:222
+  ret i32 %ret
+}
+
----------------
efriedma wrote:
> efriedma wrote:
> > efriedma wrote:
> > > I'm pretty sure this transform is wrong.  For example, `@capture` could create a mutex protecting `%local`, and lock it.  Then `@opaque` could release the mutex, does some work, and lock the mutex.  In that case, the value stored by `store i32 %x2, i32* %local` is visible to other threads in each iteration, so you can't sink the store out of the loop.
> > Err, my example is overly complicated.  You don't need multiple threads.  The address of `%local` escapes, therefore `@opaque` could read it, therefore we have to update it every iteration of the loop.
> Err, sorry, ignore me, didn't notice the argmemonly annotation on `@opaque`.  I need to think a bit.
I'm not actually changing this behavior.
The transformation is already performed when @opaque is marked as nounwind. This patch only removes the nounwind requirement, which has no bearing on your scenario.

In any case, I don't think the stores are required to be observable in this scenario (and you also need to pass the mutex as an argument, but, technically, that's possible), so I believe the transformation is still legal. If we reach the conclusion it's not, then it will still be legal if opaque is "argmemonly readonly", since readonly functions are not allowed to perform atomic writes. So we'll need to start requiring readonly (and not just argmemonly), but that's orthogonal to this patch.



https://reviews.llvm.org/D28147