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

[Philip Reames via Phabricator via llvm-commits]

reames requested changes to this revision.
reames added a comment.
This revision now requires changes to proceed.

Sorry for the late reply.  Replying selectively to key points.

In D61652#1520287 <https://reviews.llvm.org/D61652#1520287>, @jdoerfert wrote:

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


The main thing is your missing the wording about restrictions on where the attribute can be applied (i.e. pointer types).  Reading the existing dereferenceable_or_null wording more careful, we have precedent for just assuming it carries over to other dereferenceable* flavours, so I'll drop this objection.  I may separately post a patch to refine the wording, but this is not a key issue.

Now, my remaining concern.  This patch, as structured, introduces a number of purely definitional miscompiles into LLVM.  There are existing transformations which assume the dereferenceable global property for correctness.  By introducing a new attribute with the old semantics, and not updating all of the existing code to use the new attribute (code, tests, etc..), you're are defining (via the langref change) existing optimizations to be incorrect.

I really don't think that's okay.  Particular, since it should be very easy to fix.  A simple find replace in source and tests which replaces checks for dereferenceable with checks for dereferenceable_globally should be all that's needed.

I'm specifically asking that this change not be committed - despite Hal's LGTM - before this point is addressed.

p.s. I wouldn't be thrilled by it, but I'd even be okay with a clear statement of intent to handle this in follow up changes.  It is more of a theoretical problem than a practical one in the near term.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61652