[PATCH] D158081: [IR] Add writable attribute

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/docs/LangRef.rst:1537
+    This attribute is only meaningful in conjunction with ``dereferenceable(N)``
+    or another attribute that implies the first ``N`` bytes of the pointer
+    argument are dereferenceable.
----------------
efriedma wrote:
> nikic wrote:
> > efriedma wrote:
> > > nikic wrote:
> > > > efriedma wrote:
> > > > > I think I'd suggest dropping the "or another attribute" part; it makes it harder to reason about the semantics on the caller side.
> > > > My motivation for this is to allow reusing standard dereferenceability APIs. Otherwise we would have to thread an extra option through APIs like isDereferenceablePointer() that restricts it to only dereferenceable and excludes the other attributes.
> > > Value::getPointerDereferenceableBytes() isn't that complicated; we can write out the other cases it handles here, I guess.  I'm more worried about it being open-ended.
> > > 
> > > To be more specific about what I'm worried about, the exact number of bytes written is relevant for a couple of reasons.  One, alias analysis in the caller: ideally, the caller should be able to understand which bytes are modified (although I guess that might require attributes that don't currently exist).  And two, any transform that modifies a relevant attribute needs to worry about whether the transform invalidates the "writable" attribute.
> > I've added an explicit list of the attributes.
> > 
> > I think in terms of inference the case to be concerned about is when `dereferenceable` with a larger byte range is inferred (maybe Attributor does that, haven't checked). The rest are ABI attributes and we shouldn't infer those.
> > 
> > How would the caller use `writable` for alias analysis?
> The presence of a non-atomic write allows other non-atomic writes to be introduced; LICM could use that in theory.  Maybe too niche to really be useful.
> 
> At some point, we could add an attribute on calls to indicate the maximum number of bytes written through an argument; if we have that, making sure we don't write past the limit would become more important.
> maximum number of bytes
We never landed objectsize, which is similar, and 'maxwrite' would also be good, esp. now that we can shrink allocations and track content of memory.



================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:7918
+    // Clear conflicting writable attribute.
+    if (isAssumedReadNone() || isAssumedReadOnly())
+      A.removeAttrs(IRP, Attribute::Writable);
----------------
one implies the other. Maybe use a precompiled set, like AttrKinds above, the call to remove is not free.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:8107
     A.removeAttrs(getIRPosition(), AttrKinds);
+    // FIXME: Remove writable attribute if necessary.
     return A.manifestAttrs(getIRPosition(),
----------------
nikic wrote:
> @jdoerfert What would be the correct way to remove an attribute from all parameters here?
> 
> In practice just the handling above seems to be enough, but it would be cleaner to explicitly remove writable if we infer readonly here.
You can just add it to `AttrKinds` that is passed to `removeAttrs`, no?


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

https://reviews.llvm.org/D158081