[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 16:44:23 PST 2016


On Sat, Dec 31, 2016 at 4:12 PM, Sanjoy Das <sanjoy at playingwithpointers.com>
wrote:

> Hi Daniel,
>
> On Sat, Dec 31, 2016 at 3:47 PM, Daniel Berlin via llvm-commits
> <llvm-commits at lists.llvm.org> wrote:
> > 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].
>
> The hair I'd split here is that the langref specifically talks about
> "C++ exception throwing methods" (i.e. __cxa_throw etc.), but it does
> not disallow other non-memory-writing non-C++ unwind mechanisms.  For
> instance, @f is inferred to be readnone by LLVM currently, despite the
> fact that it throws.
>
> define void @f() personality i8 42 {
> entry:
>   resume i32 0
> }
>
>
1. Note that this seems to disagree with what phillip said on this thread:
"
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."

2. I have no problem with us with deciding what we mean in either direction.
I just believe it's worth having a real mailing list discussion about it
not buried in this particular thread, before making such a patch.

> 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. :)
>
> I think the patch (rL2907940) is a good idea even if we decide that
> readnone / readonly do imply nounwind -- that sort of policy should live
> either in Instruction::doesNotThrow or we should have function attrs
> or something like that mark readnone / readonly functions as nounwind,
> instead of having it scattered all over LLVM.
>

Sure, but we shouldn't lose optimization to do it.
If the answer is "these things are nounwind", i would have expected such a
refactoring to do what you suggest, not just "turn it off"  :)

If the answer is "these things are not nounwind", i would have expected
such a change to update the langref so it doesn't split hairs and have,
IMHO, inconsistent wording. :)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161231/ed088048/attachment.html>


More information about the llvm-commits mailing list