[PATCH] D61652: [LangRef][Attr] Clarify dereferenceable(_in_scope)

Tue May 28 17:22:27 PDT 2019

jdoerfert added a comment.

In D61652#1520190 <https://reviews.llvm.org/D61652#1520190>, @reames wrote:

> In D61652#1504840 <https://reviews.llvm.org/D61652#1504840>, @jdoerfert wrote:
>
> > In D61652#1504093 <https://reviews.llvm.org/D61652#1504093>, @reames wrote:
> >
> > > I think the framing of this in terms of scopes is likely to be problematic.  The current definition of dereferenceable is (I think) *stronger* than a simple assertion of the dereferenceability within a particular scope.  As an example, consider:
> > >  call void @foo(i8* dereferenceable(8) %p)
> > >  call void @may_trap_but_not_write()
> > >  %val = load i64, i64 @G
> > >
> > > In this example, the current semantic seem to allow the load to be lifted above the second call, even though it is *not* contained within any scope marked dereferenceable.    Now, that's my understanding of the current wording, but I have to admit, I can't find an optimization which actually does that style of reasoning, so maybe I'm mistaken.
> >
> >
> > The discussion with @sanjoy lead to the change from `scope` to `globally`, maybe that resolves this?
>
>
> It does.  The globally wording seems broad enough.  You're missing a bunch of detail from the original spec which needs to be preserved, but the basic notion seems reasonable.

Please elaborate on the details I'm missing, I want to get this "right". Even if we do not take this approach but the opposite one, I think we should clarify everything as good as possible. I say this also because I personally do not think the Clang interpretation violates the current wording of `dereferenceable`. While the intention is not 100% known to me [1], the wording right now is vague enough to be interpreted either way, or maybe even some other way. With the proposed patch, both versions have the same wording "a pointer is dereferenceable" as the current attribute has. The meaning is disambiguated only due to the added specifiers.

[1] According to r213385, the commit that introduced the attribute, C++ references were a use case. This would make the Clang interpretation the intended one. Though, the problems that come with that interpretation were not discussed/considered at that time.

>>> Switching topics, I have a much more fundamental objection to your patch which comes in two parts.
>>> 
>>> Part 1 - By introducing a new attribute and not switching *all* users within the optimizer over, you are implicitly a) causing a bunch of miscompiles and b) setting up a situation where optimizations are likely to be weakened to "fix" bugs.   This to me is a showstopper with the current patch, and on this basis I *strongly object* to this landing without this being addressed.
>> 
>> The situation right now is that LLVM is **broken**. The front-end and middle-end disagree over the definition of `dereferenceable`. That is a problem because we cannot even "fix" because we don't know what part is "right" and what part is "wrong".
> 
> Ok, no.  I'm going to have to be really pedantic about this.  *Clang* might be broken, but LLVM is not.  Clang is simple one of several frontends which target LLVM.  If Clang chose to ignore the documented meaning of an attribute, well, that's too bad for Clang.

I guess my comment from above represents my view point. Though, I actually do not care who is right and wrong (see below).

> To be clear here, I am not objecting to the addition of an attribute which fits the behaviour Clang wants.  I am *strongly* objecting to the notion that it's okay to break other frontends which got it right because Clang didn't.  To be very explicit, this patch as currently posted would badly break our Java frontend.
> 
> I would strongly suggest that you leave the existing attribute with the existing meaning and introduce a new dereferenceable_at_entry attribute with your desired semantics.  I won't actively object to introducing a new attribute with the prior semantics and requiring a rename for frontends which used the old one correctly, but even that feels distinctly hostile to other frontends.  It's not an acute issue for us, so I won't fight it actively, but the attitude it conveys is not exactly welcoming for new frontends.

By making the existing attribute spelling less powerful I take away optimization opportunities for front-ends until they moved to `deref_globally` but I don't see how that would break them.
I proposed this solution not to make "Clang correct" but because I think it does not "break" anything. Doing it the other way around, however, would make old Clang code definitively invalid.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61652/new/

https://reviews.llvm.org/D61652