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

[Björn Steinbrink]

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.

Cheers,
Björn

>>
>> 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.
>> +        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
>
>