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

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

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.

>> 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.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61652