[llvm] Removing writeonly and readnone incompatibility.

[homerdin]

Summary

It seems writeonly is a subset of readnone and are compatible. This corresponds to the beginning of the documentation on readnone.

On a function, this attribute indicates that the function computes its result (or decides to unwind an exception) based strictly on its arguments, without dereferencing any pointer arguments or otherwise accessing any mutable state (e.g. memory, control registers, etc) visible to caller functions.

However the next part of readnone's documentation seems unrelated to the readnone attribute and would conflict with writeonly.

It does not write through any pointer arguments (including byval arguments) and never changes any state visible to callers.

This patch would remove the conflict between writeonly and readnone.

[reames]

This patch makes no sense. The LangRef text is completely reasonable as far as I can tell. What is your actual intent here? What problem are you trying to solve?

To be very explicit, this patch should not land without either a sign off from me or another knowledgeable reviewer.

[homerdin]

Thanks for the review, I should have provided my intent from the beginning.

I am working to enable functions that only write to memory to be hoisted out of loops and to combined by EarlyCSE, more specifically I am working to mark the math functions that only write to errno as writeonly then have them handled as writeonly functions.

It seems I was mislead by the name readnone, when it is intended as read none and write no visible state.

I will abandon this change and look to see why functions are being marked as both writeonly and readnone with my changes when the function writes.

Any thoughts on how we can enable the changes I am working towards would be appreciated.

[reames]

In D46240#1083286, @homerdin wrote:

I am working to enable functions that only write to memory to be hoisted out of loops and to combined by EarlyCSE, more specifically I am working to mark the math functions that only write to errno as writeonly then have them handled as writeonly functions.

You'll need something a bit stronger than an aliasing property. You're going to specifically need to model *what* is written to errno. At the moment, we don't have a good way to model a function which writes only to one specific global (errno) or to model what those writes are.

One thing you could explore is to build in "DSE" for builtin functions. Once you got a basic design worked out in rough patch form, we could look to see what attributes might make sense to extract.

It seems I was mislead by the name readnone, when it is intended as read none and write no visible state.

readnone is intended to be the intersection of readonly and writeonly. Naming wise, readnone pre-existed the existence of writeonly.

I will abandon this change and look to see why functions are being marked as both writeonly and readnone with my changes when the function writes.

Any thoughts on how we can enable the changes I am working towards would be appreciated.

See above. I'd start this by working with builtin's via TLI and DSE, then go from there.

[hfinkel]

In D46240#1083669, @reames wrote:

In D46240#1083286, @homerdin wrote:

I am working to enable functions that only write to memory to be hoisted out of loops and to combined by EarlyCSE, more specifically I am working to mark the math functions that only write to errno as writeonly then have them handled as writeonly functions.

You'll need something a bit stronger than an aliasing property. You're going to specifically need to model *what* is written to errno. At the moment, we don't have a good way to model a function which writes only to one specific global (errno) or to model what those writes are.

I'm not sure that's true. If a function only writes, then what it writes must always be a function only of its arguments. So if I have:

foo(int, int*) writeonly;

foo(a, b);
foo(a, b);

then, I think, we can CSE these calls to foo. foo might write to its second argument somewhere, and might write to globals, but since the arguments are the same (and it doesn't read any state otherwise), it must write the same thing each time.

One thing you could explore is to build in "DSE" for builtin functions. Once you got a basic design worked out in rough patch form, we could look to see what attributes might make sense to extract.

It seems I was mislead by the name readnone, when it is intended as read none and write no visible state.

readnone is intended to be the intersection of readonly and writeonly. Naming wise, readnone pre-existed the existence of writeonly.

I will abandon this change and look to see why functions are being marked as both writeonly and readnone with my changes when the function writes.

Yea, readnone also means non-writing. I suppose the naming is now a bit unfortunate.

Any thoughts on how we can enable the changes I am working towards would be appreciated.

See above. I'd start this by working with builtin's via TLI and DSE, then go from there.

[reames]

In D46240#1083768, @hfinkel wrote:

In D46240#1083669, @reames wrote:

In D46240#1083286, @homerdin wrote:

I am working to enable functions that only write to memory to be hoisted out of loops and to combined by EarlyCSE, more specifically I am working to mark the math functions that only write to errno as writeonly then have them handled as writeonly functions.

You'll need something a bit stronger than an aliasing property. You're going to specifically need to model *what* is written to errno. At the moment, we don't have a good way to model a function which writes only to one specific global (errno) or to model what those writes are.

I'm not sure that's true. If a function only writes, then what it writes must always be a function only of its arguments. So if I have:

foo(int, int*) writeonly;

foo(a, b);
foo(a, b);

then, I think, we can CSE these calls to foo. foo might write to its second argument somewhere, and might write to globals, but since the arguments are the same (and it doesn't read any state otherwise), it must write the same thing each time.

Good point. I agree this works and likely forms a good basis for what the errno case.

docs/LangRef.rst

``"probe-stack"``
     function that has a ``"probe-stack"`` attribute is inlined into a
     function that has no ``"probe-stack"`` attribute at all, the resulting
     function has the ``"probe-stack"`` attribute of the callee.
 ``readnone``
     On a function, this attribute indicates that the function computes its
     result (or decides to unwind an exception) based strictly on its arguments,
     without dereferencing any pointer arguments or otherwise accessing
     any mutable state (e.g. memory, control registers, etc) visible to
-    caller functions. It does not write through any pointer arguments
-    (including ``byval`` arguments) and never changes any state visible
-    to callers. This means while it cannot unwind exceptions by calling
+    caller functions. This means while it cannot unwind exceptions by calling
     the ``C++`` exception throwing methods (since they write to memory), there may
     be non-``C++`` mechanisms that throw exceptions without writing to LLVM
     visible memory.
 
     On an argument, this attribute indicates that the function does not
     dereference that pointer argument, even though it may read or write the
     memory that the pointer points to if accessed through other pointers.
 ``readonly``

lib/IR/Verifier.cpp

Assert(!(Attrs.hasAttribute(Attribute::ZExt) &&
          V);
 
   Assert(!(Attrs.hasAttribute(Attribute::ReadNone) &&
            Attrs.hasAttribute(Attribute::ReadOnly)),
          "Attributes "
          "'readnone and readonly' are incompatible!",
          V);
 
-  Assert(!(Attrs.hasAttribute(Attribute::ReadNone) &&
-           Attrs.hasAttribute(Attribute::WriteOnly)),
-         "Attributes "
-         "'readnone and writeonly' are incompatible!",
-         V);
-
   Assert(!(Attrs.hasAttribute(Attribute::ReadOnly) &&
            Attrs.hasAttribute(Attribute::WriteOnly)),
          "Attributes "
          "'readonly and writeonly' are incompatible!",
          V);
 
   Assert(!(Attrs.hasAttribute(Attribute::NoInline) &&
            Attrs.hasAttribute(Attribute::AlwaysInline)),
if (!Attrs.hasAttributes(AttributeList::FunctionIndex))
     return;
 
   verifyAttributeTypes(Attrs.getFnAttributes(), /*IsFunction=*/true, V);
 
   Assert(!(Attrs.hasFnAttribute(Attribute::ReadNone) &&
            Attrs.hasFnAttribute(Attribute::ReadOnly)),
          "Attributes 'readnone and readonly' are incompatible!", V);
 
-  Assert(!(Attrs.hasFnAttribute(Attribute::ReadNone) &&
-           Attrs.hasFnAttribute(Attribute::WriteOnly)),
-         "Attributes 'readnone and writeonly' are incompatible!", V);
-
   Assert(!(Attrs.hasFnAttribute(Attribute::ReadOnly) &&
            Attrs.hasFnAttribute(Attribute::WriteOnly)),
          "Attributes 'readonly and writeonly' are incompatible!", V);
 
   Assert(!(Attrs.hasFnAttribute(Attribute::ReadNone) &&
            Attrs.hasFnAttribute(Attribute::InaccessibleMemOrArgMemOnly)),
          "Attributes 'readnone and inaccessiblemem_or_argmemonly' are "
          "incompatible!",

test/Verifier/writeonly.ll

 ; RUN: not llvm-as %s -o /dev/null 2>&1 | FileCheck %s
 
-declare void @a() readnone writeonly
-; CHECK: Attributes {{.*}} are incompatible
-
 declare void @b() readonly writeonly
 ; CHECK: Attributes {{.*}} are incompatible
 
-declare void @c(i32* readnone writeonly %p)
-; CHECK: Attributes {{.*}} are incompatible
-
 declare void @d(i32* readonly writeonly %p)
 ; CHECK: Attributes {{.*}} are incompatible