This is an archive of the discontinued LLVM Phabricator instance.

Add support for nonnull metadata on Loads
ClosedPublic

Authored by reames on Sep 5 2014, 2:42 PM.

Details

Summary

This patch simply adds support for marking a load as returning a non-null pointer. This is analogous to the existing nonnull return attribute, but it applies to specific load instructions instead. The value of the load is allowed to vary between successive loads, but null is not a valid value to be loaded by any load marked nonnull.

Hal - I'd particularly like your input here. Is adding a parallel metadata construct to the existing attribute a reasonable idea? Should we be uniform and accept the metadata on calls too? Is there a better way to approach this.

Diff Detail

Event Timeline

reames updated this revision to Diff 13345.Sep 5 2014, 2:42 PM
reames retitled this revision from to Add support for nonnull metadata on Loads.
reames updated this object.
reames edited the test plan for this revision. (Show Details)
reames added a reviewer: hfinkel.
reames added a subscriber: Unknown Object (MLST).
hfinkel edited edge metadata.Sep 8 2014, 4:32 AM
  • Original Message -----

From: "Philip Reames" <listmail@philipreames.com>
To: listmail@philipreames.com, hfinkel@anl.gov
Cc: llvm-commits@cs.uiuc.edu
Sent: Friday, September 5, 2014 4:42:25 PM
Subject: [PATCH] Add support for nonnull metadata on Loads

Hi hfinkel,

This patch simply adds support for marking a load as returning a
non-null pointer. This is analogous to the existing nonnull return
attribute, but it applies to specific load instructions instead.
The value of the load is allowed to vary between successive loads,
but null is not a valid value to be loaded by any load marked
nonnull.

Hal - I'd particularly like your input here. Is adding a parallel
metadata construct to the existing attribute a reasonable idea?
Should we be uniform and accept the metadata on calls too? Is
there a better way to approach this.

Now that the @llvm.assume infrastructure is in place, I can legitimately respond by saying: I think that we can model this by @llvm.assume(x != null). If that currently does not work, then we should fix it.

Thanks again,
Hal

http://reviews.llvm.org/D5220

Files:

lib/Analysis/ValueTracking.cpp
test/Transforms/InstSimplify/compare.ll

It would be an entirely new thing, but I would rather this be a
non-metadata attribute on loads, and support nonnull as well as
dereferenceable(n) and possibly other pointer attributes in the future.

reames updated this revision to Diff 13840.Sep 18 2014, 11:05 AM
reames edited edge metadata.

Adding LangRef changes.

assume(x != nullptr) will follow in a separate review thread.

Also, if I might engage in some bikeshedding, I'd prefer that we name this something other than just nonnull, because I'd like to make it more clear that this refers to the value being returned from the load and not the pointer from which the value is being loaded. Maybe result.nonnull or value.nonnull or similar?

docs/LangRef.rst
5223 ↗(On Diff #13840)

I'd prefer to specify this in the same way that !invariant.load is specified. It says:

The optional ``!invariant.load`` metadata must reference a single
metadata name ``<index>`` corresponding to a metadata node with no 
entries. The existence of the ``!invariant.load`` metadata on the
instruction tells the optimizer...

I'd like to use similar terminology here to talk about what the referenced metadata should be (instead of saying unspecified and meaningless).

reames updated this revision to Diff 15091.Oct 17 2014, 11:44 AM

Finally got around to rephrasing the LangRef as Hal requested.

I chose not to adopt the name result.nonnull. I believe we should stick with precedent here. There's an existing nonnull attribute with the same semantics. I believe using an alternate name would be more confusing.

Hal, with a commitment to handle assume(x != null) in the near future, is this patch good to go?

I chose not to adopt the name result.nonnull. I believe we should stick with precedent here. There's an existing nonnull attribute with the same semantics. I believe using an alternate name would be more confusing.

I hate to be a pain, but -- now that I've had time to think about it ;) -- I *really* would like to name this something that makes it clear that the semantics apply to the result, and not the operand -- we *don't* currently have a naming precedent here (for metadata on loads that mirrors argument attributes), and I'd like to set a precedent that makes sense, not one that will lead to confusion. The existing attributes are clearly attached to values, so there is no ambiguity. But here there is a large ambiguity: there are two pointers involved here, and seeing !nonnull next to a load, it would be perfectly reasonable to assume that meant that the address being loaded from is nonnull (and not that the pointer being loaded is nonnull). Let's pick a less-ambiguous name and I think this is good. Let's set a precedent for result.<attr-name> or value.<attr-name> or whatever, I think that will be much better.

In D5220#12, @hfinkel wrote:

I chose not to adopt the name result.nonnull. I believe we should stick with precedent here. There's an existing nonnull attribute with the same semantics. I believe using an alternate name would be more confusing.

I hate to be a pain, but -- now that I've had time to think about it ;) -- I *really* would like to name this something that makes it clear that the semantics apply to the result, and not the operand -- we *don't* currently have a naming precedent here (for metadata on loads that mirrors argument attributes), and I'd like to set a precedent that makes sense, not one that will lead to confusion. The existing attributes are clearly attached to values, so there is no ambiguity. But here there is a large ambiguity: there are two pointers involved here, and seeing !nonnull next to a load, it would be perfectly reasonable to assume that meant that the address being loaded from is nonnull (and not that the pointer being loaded is nonnull). Let's pick a less-ambiguous name and I think this is good. Let's set a precedent for result.<attr-name> or value.<attr-name> or whatever, I think that will be much better.

I would appreciate inputs from others on this point.

I don't agree with Hal's points. I don't really see there being much confusion - all metadata and most attributes apply to the value of the instruction, not the operands - and I believe that having two names for the exact same semantics (one attribute, one metadata) is confusing.

If outvoted, I will go along with the majority here, but I believe the naming scheme Hal is proposing would be a mistake.

  • Original Message -----

From: "Philip Reames" <listmail@philipreames.com>
To: listmail@philipreames.com, hfinkel@anl.gov
Cc: nlewycky@google.com, llvm-commits@cs.uiuc.edu
Sent: Friday, October 17, 2014 4:28:58 PM
Subject: Re: [PATCH] Add support for nonnull metadata on Loads

In D5220#12, @hfinkel wrote:

I chose not to adopt the name result.nonnull. I believe we should
stick with precedent here. There's an existing nonnull attribute
with the same semantics. I believe using an alternate name would
be more confusing.

I hate to be a pain, but -- now that I've had time to think about
it ;) -- I *really* would like to name this something that makes
it clear that the semantics apply to the result, and not the
operand -- we *don't* currently have a naming precedent here (for
metadata on loads that mirrors argument attributes), and I'd like
to set a precedent that makes sense, not one that will lead to
confusion. The existing attributes are clearly attached to values,
so there is no ambiguity. But here there is a large ambiguity:
there are two pointers involved here, and seeing !nonnull next to
a load, it would be perfectly reasonable to assume that meant that
the address being loaded from is nonnull (and not that the pointer
being loaded is nonnull). Let's pick a less-ambiguous name and I
think this is good. Let's set a precedent for result.<attr-name>
or value.<attr-name> or whatever, I think that will be much
better.

I would appreciate inputs from others on this point.

I don't agree with Hal's points. I don't really see there being much
confusion - all metadata and most attributes apply to the value of
the instruction, not the operands - and I believe that having two
names for the exact same semantics (one attribute, one metadata) is
confusing.

I'm not proposing two different names, I'm proposing a prefix that is used on the metadata name. Many metadata names have prefixes. Also, I reject your premise:

!tbaa, !noalias/!alias.scope, !llvm.mem.parallel_loop_access, all apply to loads, and all refer to properties of the pointer being loaded from. Only !range applies to loads and refers to the value being loaded, and I don't see any particular ambiguity there. There are more types of metadata in the first category than in the second (!invariant.load could, perhaps, be argued either way, so I left it out, but I also view it as more of a loaded-from-pointer property than a property of the value being loaded).

If outvoted, I will go along with the majority here, but I believe
the naming scheme Hal is proposing would be a mistake.

Sounds good. ;)

Thanks again,
Hal

http://reviews.llvm.org/D5220

reames closed this revision.Oct 20 2014, 3:51 PM
reames updated this revision to Diff 15157.

Closed by commit rL220240 (authored by @reames).