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

Daniel Berlin via llvm-commits llvm-commits at lists.llvm.org
Sat Dec 31 15:47:13 PST 2016


What about readnone?
The last time i asked, i got the answer that readnone implied nounwind.

Also, the langref currently says

readnone:

...
It does not write through any pointer arguments (including byval arguments)
and never changes any state visible to callers. This means that it cannot
unwind exceptions by calling the C++ exception throwing methods.

readonly:
...
A readonly function always returns the same value (or unwinds an exception
identically) when called with the same set of arguments and global state.
It cannot unwind an exception by calling the C++ exception throwing
methods...


I'm not an expert in this area by any means, but if i was to read this, i
would think it implied nounwind[1].
So if that's not what we mean, we should change it :)

Thus, IMHO this patch deserves a bit of discussion, with the  answer to
whether they are nounwind being explicitly written down in the langref in a
way dumb people like me can understand it. :)

--Dan

[1]   Note also the hilarious contradiction of readonly saying it unwinds
an exception identically but then saying it can't unwind an exception :)


On Sat, Dec 31, 2016 at 2:25 PM, Sanjoy Das via Phabricator via
llvm-commits <llvm-commits at lists.llvm.org> wrote:

> sanjoy 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.
>
>
> That was an oversight, fixed in https://reviews.llvm.org/rL290794
>
> > 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.
>
>
>
>
> https://reviews.llvm.org/D28147
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161231/1b566427/attachment.html>


More information about the llvm-commits mailing list