[PATCH] Add C/C++ attribute 'optnone'
Aaron Ballman
aaron at aaronballman.com
Wed Nov 20 05:29:51 PST 2013
Patch LGTM! Thanks!
~Aaron
On Tue, Nov 19, 2013 at 8:56 PM, Robinson, Paul
<Paul_Robinson at playstation.sony.com> wrote:
> Revised patch with requested changes to allow 'optnone' on an
> ObjC method.
> --paulr
>
>> -----Original Message-----
>> From: aaron.ballman at gmail.com [mailto:aaron.ballman at gmail.com] On Behalf
>> Of Aaron Ballman
>> Sent: Tuesday, November 19, 2013 5:11 PM
>> To: Robinson, Paul
>> Cc: cfe-commits at cs.uiuc.edu cfe
>> Subject: Re: [PATCH] Add C/C++ attribute 'optnone'
>>
>> On Tue, Nov 19, 2013 at 6:33 PM, Robinson, Paul
>> <Paul_Robinson at playstation.sony.com> wrote:
>> >> -----Original Message-----
>> >> From: aaron.ballman at gmail.com [mailto:aaron.ballman at gmail.com] On
>> Behalf
>> >> Of Aaron Ballman
>> >> Sent: Tuesday, November 19, 2013 2:36 PM
>> >> To: Robinson, Paul
>> >> Cc: cfe-commits at cs.uiuc.edu cfe
>> >> Subject: Re: [PATCH] Add C/C++ attribute 'optnone'
>> >>
>> >> > Index: include/clang/Basic/Attr.td
>> >> > ===================================================================
>> >> > --- include/clang/Basic/Attr.td (revision 195153)
>> >> > +++ include/clang/Basic/Attr.td (working copy)
>> >> > @@ -613,6 +613,11 @@
>> >> > let Subjects = [ObjCInterface];
>> >> > }
>> >> >
>> >> > +def OptimizeNone : InheritableAttr {
>> >> > + let Spellings = [GNU<"optnone">, CXX11<"clang", "optnone">];
>> >> > + let Subjects = [Function];
>> >>
>> >> Can it apply to ObjC method declarations as well, or just functions?
>> >
>> > Um. Hadn't thought about it as ObjC isn't my use case. It can apply
>> > to C++ methods, so I guess it could? What do I do to make that work?
>>
>> In Attr.td, add ObjCMethod to the list of subjects, and in
>> SemaDeclAttr.cpp, check for ObjCMethodDecl alongside of the existing
>> FunctionDecl (and switch to using ExpectedFunctionOrMethod). I would
>> guess this attribute does apply to ObjC methods, so this strikes me as
>> a reasonable extension.
>> >
>> >>
>> >> > +}
>> >> > +
>> >> > def Overloadable : Attr {
>> >> > let Spellings = [GNU<"overloadable">];
>> >> > }
>> >> > Index: lib/CodeGen/CodeGenModule.cpp
>> >> > ===================================================================
>> >> > --- lib/CodeGen/CodeGenModule.cpp (revision 195153)
>> >> > +++ lib/CodeGen/CodeGenModule.cpp (working copy)
>> >> > @@ -678,6 +678,10 @@
>> >> > // Naked implies noinline: we should not be inlining such
>> >> functions.
>> >> > B.addAttribute(llvm::Attribute::Naked);
>> >> > B.addAttribute(llvm::Attribute::NoInline);
>> >> > + } else if (D->hasAttr<OptimizeNoneAttr>()) {
>> >> > + // OptimizeNone implies noinline; we should not be inlining
>> such
>> >> functions.
>> >> > + B.addAttribute(llvm::Attribute::OptimizeNone);
>> >> > + B.addAttribute(llvm::Attribute::NoInline);
>> >> > } else if (D->hasAttr<NoInlineAttr>()) {
>> >> > B.addAttribute(llvm::Attribute::NoInline);
>> >> > } else if ((D->hasAttr<AlwaysInlineAttr>() ||
>> >> > @@ -696,6 +700,12 @@
>> >> > if (D->hasAttr<MinSizeAttr>())
>> >> > B.addAttribute(llvm::Attribute::MinSize);
>> >> >
>> >> > + if (D->hasAttr<OptimizeNoneAttr>()) {
>> >> > + // OptimizeNone wins over OptimizeForSize and MinSize.
>> >> > + B.removeAttribute(llvm::Attribute::OptimizeForSize);
>> >> > + B.removeAttribute(llvm::Attribute::MinSize);
>> >>
>> >> Is this something we should warn the user on when we encounter a
>> >> minsize attribute along with an optnone attribute? Or should this be
>> >> mutually exclusive like in the case of optnone and alwaysinline?
>> >
>> > See reply to Richard.
>> >
>> >>
>> >> > + }
>> >> > +
>> >> > if (LangOpts.getStackProtector() == LangOptions::SSPOn)
>> >> > B.addAttribute(llvm::Attribute::StackProtect);
>> >> > else if (LangOpts.getStackProtector() == LangOptions::SSPReq)
>> >> > Index: lib/Sema/SemaDeclAttr.cpp
>> >> > ===================================================================
>> >> > --- lib/Sema/SemaDeclAttr.cpp (revision 195153)
>> >> > +++ lib/Sema/SemaDeclAttr.cpp (working copy)
>> >> > @@ -1764,6 +1764,12 @@
>> >> > return;
>> >> > }
>> >> >
>> >> > + if (D->hasAttr<OptimizeNoneAttr>()) {
>> >> > + S.Diag(Attr.getLoc(), diag::err_attributes_are_not_compatible)
>> >> > + << Attr.getName() << "'optnone'";
>> >>
>> >> This is not a grumble for you to fix, but it would be very nice if we
>> >> could get the original spelling for this diagnostic instead of
>> >> hard-coding one particular spelling.
>> >
>> > Ayup.
>> >
>> >>
>> >> > + return;
>> >> > + }
>> >> > +
>> >> > D->addAttr(::new (S.Context)
>> >> > AlwaysInlineAttr(Attr.getRange(), S.Context,
>> >> >
>> Attr.getAttributeSpellingListIndex()));
>> >> > @@ -3637,6 +3643,25 @@
>> >> > Attr.getAttributeSpellingListIndex()));
>> >> > }
>> >> >
>> >> > +static void handleOptimizeNoneAttr(Sema &S, Decl *D,
>> >> > + const AttributeList &Attr) {
>> >> > + if (!isa<FunctionDecl>(D)) {
>> >> > + S.Diag(Attr.getLoc(), diag::warn_attribute_wrong_decl_type)
>> >> > + << Attr.getName() << ExpectedFunction;
>> >> > + return;
>> >> > + }
>> >> > +
>> >> > + if (D->hasAttr<AlwaysInlineAttr>()) {
>> >> > + S.Diag(Attr.getLoc(), diag::err_attributes_are_not_compatible)
>> >> > + << Attr.getName() << "'always_inline'";
>> >> > + return;
>> >> > + }
>> >> > +
>> >> > + D->addAttr(::new (S.Context)
>> >> > + OptimizeNoneAttr(Attr.getRange(), S.Context,
>> >> > +
>> Attr.getAttributeSpellingListIndex()));
>> >> > +}
>> >> > +
>> >> > static void handleNoInstrumentFunctionAttr(Sema &S, Decl *D,
>> >> > const AttributeList
>> &Attr)
>> >> {
>> >> > if (!isa<FunctionDecl>(D)) {
>> >> > @@ -4662,6 +4687,9 @@
>> >> > case AttributeList::AT_MinSize:
>> >> > handleMinSizeAttr(S, D, Attr);
>> >> > break;
>> >> > + case AttributeList::AT_OptimizeNone:
>> >> > + handleOptimizeNoneAttr(S, D, Attr);
>> >> > + break;
>> >> > case AttributeList::AT_Format: handleFormatAttr (S, D,
>> >> Attr); break;
>> >> > case AttributeList::AT_FormatArg: handleFormatArgAttr (S, D,
>> >> Attr); break;
>> >> > case AttributeList::AT_CUDAGlobal: handleGlobalAttr (S, D,
>> >> Attr); break;
>> >> > Index: test/CodeGen/attr-optnone.c
>> >> > ===================================================================
>> >> > --- test/CodeGen/attr-optnone.c (revision 0)
>> >> > +++ test/CodeGen/attr-optnone.c (revision 0)
>> >> > @@ -0,0 +1,26 @@
>> >> > +// RUN: %clang_cc1 -fms-compatibility -emit-llvm < %s > %t
>> >> > +// RUN: FileCheck %s --check-prefix=PRESENT < %t
>> >> > +// RUN: FileCheck %s --check-prefix=ABSENT < %t
>> >> > +
>> >> > +__attribute__((optnone)) __forceinline
>> >> > +int test2() { return 0; }
>> >> > +// PRESENT-DAG: @test2{{.*}}[[ATTR2:#[0-9]+]]
>> >> > +
>> >> > +__attribute__((optnone)) __attribute__((minsize))
>> >> > +int test3() { return 0; }
>> >> > +// PRESENT-DAG: @test3{{.*}}[[ATTR3:#[0-9]+]]
>> >> > +
>> >> > +__attribute__((optnone)) __attribute__((cold))
>> >> > +int test4() { return test2(); }
>> >> > +// PRESENT-DAG: @test4{{.*}}[[ATTR4:#[0-9]+]]
>> >> > +// Also check that test2 isn't inlined into test4 (optnone trumps
>> >> forceinline).
>> >> > +// PRESENT-DAG: call i32 @test2
>> >> > +
>> >> > +// Check that we set both attributes noinline and optnone on each
>> >> function.
>> >> > +// PRESENT-DAG: attributes [[ATTR2]] = {
>> >> {{.*}}noinline{{.*}}optnone{{.*}} }
>> >> > +// PRESENT-DAG: attributes [[ATTR3]] = {
>> >> {{.*}}noinline{{.*}}optnone{{.*}} }
>> >> > +// PRESENT-DAG: attributes [[ATTR4]] = {
>> >> {{.*}}noinline{{.*}}optnone{{.*}} }
>> >> > +
>> >> > +// Check that no 'optsize' or 'minsize' attributes appear.
>> >> > +// ABSENT-NOT: optsize
>> >> > +// ABSENT-NOT: minsize
>> >> > Index: test/SemaCXX/attr-optnone.cpp
>> >> > ===================================================================
>> >> > --- test/SemaCXX/attr-optnone.cpp (revision 0)
>> >> > +++ test/SemaCXX/attr-optnone.cpp (revision 0)
>> >> > @@ -0,0 +1,44 @@
>> >> > +// RUN: %clang_cc1 -std=c++11 -fms-compatibility -fsyntax-only -
>> >> verify %s
>> >> > +
>> >> > +int foo() __attribute__((optnone));
>> >> > +int bar() __attribute__((optnone)) __attribute__((noinline));
>> >> > +
>> >> > +int baz() __attribute__((always_inline)) __attribute__((optnone));
>> //
>> >> expected-error{{'always_inline' and 'optnone' attributes are not
>> >> compatible}}
>> >> > +int qux() __attribute__((optnone)) __attribute__((always_inline));
>> //
>> >> expected-error{{'optnone' and 'always_inline' attributes are not
>> >> compatible}}
>> >> > +
>> >> > +int globalVar __attribute__((optnone)); // expected-
>> >> warning{{'optnone' attribute only applies to functions}}
>> >> > +
>> >> > +int fubar(int __attribute__((optnone)), int); // expected-
>> >> warning{{'optnone' attribute only applies to functions}}
>> >> > +
>> >> > +struct A {
>> >> > + int aField __attribute__((optnone)); // expected-
>> >> warning{{'optnone' attribute only applies to functions}}
>> >> > +};
>> >> > +
>> >> > +struct B {
>> >> > + void foo() __attribute__((optnone));
>> >> > + static void bar() __attribute__((optnone));
>> >> > +};
>> >> > +
>> >> > +// Verify that we can specify the [[clang::optnone]] syntax as
>> well.
>> >> > +
>> >> > +[[clang::optnone]]
>> >> > +int foo2();
>> >> > +[[clang::optnone]]
>> >> > +int bar2() __attribute__((noinline));
>> >> > +
>> >> > +[[clang::optnone]]
>> >> > +int baz2() __attribute__((always_inline)); // expected-
>> >> error{{'always_inline' and 'optnone' attributes are not compatible}}
>> >> > +
>> >> > +[[clang::optnone]] int globalVar2; //expected-warning{{'optnone'
>> >> attribute only applies to functions}}
>> >> > +
>> >> > +struct A2 {
>> >> > + [[clang::optnone]] int aField; // expected-warning{{'optnone'
>> >> attribute only applies to functions}}
>> >> > +};
>> >> > +
>> >> > +struct B2 {
>> >> > + [[clang::optnone]]
>> >> > + void foo();
>> >> > + [[clang::optnone]]
>> >> > + static void bar();
>> >> > +};
>> >> > +
>> >> >
>> >>
>> >> ~Aaron
>> >>
>> >> On Tue, Nov 19, 2013 at 5:25 PM, Robinson, Paul
>> >> <Paul_Robinson at playstation.sony.com> wrote:
>> >> > Adds a new ‘optnone’ function attribute that maps directly to the
>> LLVM
>> >> IR
>> >> > ‘optnone’ attribute.
>> >> >
>> >> > --paulr
>> >> >
>> >> >
>> >> >
>> >> >
>> >> > _______________________________________________
>> >> > cfe-commits mailing list
>> >> > cfe-commits at cs.uiuc.edu
>> >> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>> >> >
>> >
>>
>> ~Aaron
>
More information about the cfe-commits
mailing list