[PATCH] Remove conflicting attributes before adding deduced readonly/readnone

[Nick Lewycky]

Björn Steinbrink wrote:
> 2015-05-25 21:27 GMT+02:00 Nick Lewycky<nicholas at mxc.ca>:
>>
>> Björn Steinbrink wrote:
>>>
>>> Hi nlewycky,
>>>
>>> In case of functions that have a pointer argument and only pass it to
>>> each other, the function attributes pass deduces that the pointer should
>>> get the readnone attribute, but fails to remove a readonly attribute
>>> that may already have been present.
>>
>>
>> I suspect something else is wrong then; readnone implies readonly, so it should be impossible to have readnone *without* readonly. This is how it works on the function attributes, I suspect the pointer attributes may be missing similar logic to suppress printing of readonly and setting of readonly when readnone is set?
>
> The verifier checks that only one of readonly and readnone is set, and
> I actually copied the attribute removal from the code that handles the
> function attributes. In fact, the verifier also complains about this:
>
>      define void @foo() readonly readnone {
>          ret void
>      }
>
> I found this to be confusing as well, but assumed that since the
> function attributes are handled the same way, the right fix is to
> remove the conflicting attribute.

Oh. As long as testing for readonly returns true when readnone is set, 
that works.

>>> http://reviews.llvm.org/D9995
>>>
>>> Files:
>>>     lib/Transforms/IPO/FunctionAttrs.cpp
>>>     test/Transforms/FunctionAttrs/readnone.ll
>>>
>>> Index: lib/Transforms/IPO/FunctionAttrs.cpp
>>> ===================================================================
>>> --- lib/Transforms/IPO/FunctionAttrs.cpp
>>> +++ lib/Transforms/IPO/FunctionAttrs.cpp
>>> @@ -703,10 +703,14 @@
>>>        }
>>>
>>>        if (ReadAttr != Attribute::None) {
>>> -      AttrBuilder B;
>>> +      AttrBuilder B, R;
>>>          B.addAttribute(ReadAttr);
>>> +      R.addAttribute(Attribute::ReadOnly)
>>> +        .addAttribute(Attribute::ReadNone);
>>>          for (unsigned i = 0, e = ArgumentSCC.size(); i != e; ++i) {
>>>            Argument *A = ArgumentSCC[i]->Definition;
>>> +        // Clear out any existing attributes.

This only clears out readnone/readonly attributes, not all attributes.

LGTM, with a change to the comment.

Nick

>>> +        A->removeAttr(AttributeSet::get(A->getContext(), A->getArgNo() + 1, R));
>>>            A->addAttr(AttributeSet::get(A->getContext(), A->getArgNo() + 1, B));
>>>            ReadAttr == Attribute::ReadOnly ? ++NumReadOnlyArg : ++NumReadNoneArg;
>>>            Changed = true;
>>> Index: test/Transforms/FunctionAttrs/readnone.ll
>>> ===================================================================
>>> --- /dev/null
>>> +++ test/Transforms/FunctionAttrs/readnone.ll
>>> @@ -0,0 +1,13 @@
>>> +; RUN: opt<   %s -functionattrs -S | FileCheck %s
>>> +
>>> +; CHECK: define void @bar(i8* nocapture readnone)
>>> +define void @bar(i8* readonly) {
>>> +  call void @foo(i8* %0)
>>> +    ret void
>>> +}
>>> +
>>> +; CHECK: define void @foo(i8* nocapture readnone)
>>> +define void @foo(i8* readonly) {
>>> +  call void @bar(i8* %0)
>>> +  ret void
>>> +}
>>>
>>> EMAIL PREFERENCES
>>>     http://reviews.llvm.org/settings/panel/emailpreferences/
>>>
>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>>
>