[PATCH] PR21604: Correctly remove OptimizeForSize from OptimizeNone functions
Aaron Ballman
aaron at aaronballman.com
Mon Nov 24 11:06:32 PST 2014
On Mon, Nov 24, 2014 at 12:55 PM, Robinson, Paul
<Paul_Robinson at playstation.sony.com> wrote:
> Ping. Aaron, could you PTAL?
Patch LGTM, but I would like to see a follow-up patch that warns when
optnone causes other attributes the user has written to be ignored.
~Aaron
> --paulr
>
>> -----Original Message-----
>> From: cfe-commits-bounces at cs.uiuc.edu [mailto:cfe-commits-
>> bounces at cs.uiuc.edu] On Behalf Of Robinson, Paul
>> Sent: Wednesday, November 19, 2014 5:22 PM
>> To: cfe-commits at cs.uiuc.edu
>> Subject: [PATCH] PR21604: Correctly remove OptimizeForSize from
>> OptimizeNone functions
>>
>> While it might have looked like OptimizeNone removed the OptimizeForSize
>> attribute from a function, it weren't necessarily so. This patch fixes
>> the attribute handling so 'optnone' correctly supersedes all the other
>> things it's supposed to supersede.
>>
>> Updated a test to do a -Os run so we verify that part works right.
>> Added a test to exercise different combinations of attributes on the
>> declaration versus on the definition, which is another way that these
>> things can end up having to be merged/untangled.
>> --paulr
>
-------------- next part --------------
> Index: lib/CodeGen/CodeGenModule.cpp
> ===================================================================
> --- lib/CodeGen/CodeGenModule.cpp (revision 222337)
> +++ lib/CodeGen/CodeGenModule.cpp (working copy)
> @@ -715,10 +715,6 @@
> // 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<NoDuplicateAttr>()) {
> B.addAttribute(llvm::Attribute::NoDuplicate);
> } else if (D->hasAttr<NoInlineAttr>()) {
> @@ -738,12 +734,6 @@
> 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);
> - }
> -
> if (LangOpts.getStackProtector() == LangOptions::SSPOn)
> B.addAttribute(llvm::Attribute::StackProtect);
> else if (LangOpts.getStackProtector() == LangOptions::SSPStrong)
> @@ -772,6 +762,21 @@
> llvm::AttributeSet::get(
> F->getContext(), llvm::AttributeSet::FunctionIndex, B));
>
> + if (D->hasAttr<OptimizeNoneAttr>()) {
> + // OptimizeNone implies noinline; we should not be inlining such functions.
> + F->addFnAttr(llvm::Attribute::OptimizeNone);
> + F->addFnAttr(llvm::Attribute::NoInline);
> +
> + // OptimizeNone wins over OptimizeForSize, MinSize, AlwaysInline.
> + F->removeFnAttr(llvm::Attribute::OptimizeForSize);
> + F->removeFnAttr(llvm::Attribute::MinSize);
> + F->removeFnAttr(llvm::Attribute::AlwaysInline);
> +
> + // Attribute 'inlinehint' has no effect on 'optnone' functions.
> + // Explicitly remove it from the set of function attributes.
> + F->removeFnAttr(llvm::Attribute::InlineHint);
In these situations, I think the user should receive a semantic warning about these attributes being ignored. This can be done as a separate patch, as you're trying to correct behavior with this one.
> + }
> +
> if (isa<CXXConstructorDecl>(D) || isa<CXXDestructorDecl>(D))
> F->setUnnamedAddr(true);
> else if (const auto *MD = dyn_cast<CXXMethodDecl>(D))
> Index: test/CodeGen/attr-optnone.c
> ===================================================================
> --- test/CodeGen/attr-optnone.c (revision 222337)
> +++ test/CodeGen/attr-optnone.c (working copy)
> @@ -1,10 +1,13 @@
> // RUN: %clang_cc1 -emit-llvm < %s > %t
> // RUN: FileCheck %s --check-prefix=PRESENT < %t
> // RUN: FileCheck %s --check-prefix=ABSENT < %t
> +// RUN: %clang_cc1 -emit-llvm -Os < %s > %t
> +// RUN: FileCheck %s --check-prefix=PRESENT < %t
> +// RUN: FileCheck %s --check-prefix=OPTSIZE < %t
>
> __attribute__((always_inline))
> int test2() { return 0; }
> -// PRESENT-DAG: @test2{{.*}}[[ATTR2:#[0-9]+]]
> +// OPTSIZE: @test2{{.*}}[[ATTR2:#[0-9]+]]
>
> __attribute__((optnone)) __attribute__((minsize))
> int test3() { return 0; }
> @@ -23,3 +26,8 @@
> // Check that no 'optsize' or 'minsize' attributes appear.
> // ABSENT-NOT: optsize
> // ABSENT-NOT: minsize
> +
> +// With -Os, check that 'optsize' appears only on test2.
> +// OPTSIZE-NOT: optsize
> +// OPTSIZE: attributes [[ATTR2]] = { {{.*}}optsize{{.*}} }
> +// OPTSIZE-NOT: optsize
> Index: test/CodeGenCXX/optnone-def-decl.cpp
> ===================================================================
> --- test/CodeGenCXX/optnone-def-decl.cpp (revision 0)
> +++ test/CodeGenCXX/optnone-def-decl.cpp (revision 0)
> @@ -0,0 +1,94 @@
> +// RUN: %clang_cc1 %s -triple %itanium_abi_triple -fms-extensions -emit-llvm -o - | FileCheck %s
> +
> +// Test optnone on both function declarations and function definitions.
> +// Verify also that we don't generate invalid IR functions with
> +// both alwaysinline and noinline. (optnone implies noinline and wins
> +// over alwaysinline, in all cases.)
> +
> +// Test optnone on extern declaration only.
> +extern int decl_only(int a) __attribute__((optnone));
> +
> +// This function should be marked 'optnone'.
> +int decl_only(int a) {
> + return a + a + a + a;
> +}
> +
> +// CHECK: define i32 @_Z9decl_onlyi(i32 %a) [[OPTNONE:#[0-9]+]]
> +
> +// Test optnone on definition but not extern declaration.
> +extern int def_only(int a);
> +
> +__attribute__((optnone))
> +int def_only(int a) {
> + return a + a + a + a;
> +}
> +
> +// Function def_only is a optnone function and therefore it should not be
> +// inlined inside 'user_of_def_only'.
> +int user_of_def_only() {
> + return def_only(5);
> +}
> +
> +// CHECK: define i32 @_Z8def_onlyi(i32 %a) [[OPTNONE]]
> +// CHECK: define i32 @_Z16user_of_def_onlyv() [[NORMAL:#[0-9]+]]
> +
> +// Test optnone on both definition and declaration.
> +extern int def_and_decl(int a) __attribute__((optnone));
> +
> +__attribute__((optnone))
> +int def_and_decl(int a) {
> + return a + a + a + a;
> +}
> +
> +// CHECK: define i32 @_Z12def_and_decli(i32 %a) [[OPTNONE]]
> +
> +// Check that optnone wins over always_inline.
> +
> +// Test optnone on definition and always_inline on declaration.
> +extern int always_inline_function(int a) __attribute__((always_inline));
> +
> +__attribute__((optnone))
> +extern int always_inline_function(int a) {
> + return a + a + a + a;
> +}
> +// CHECK: define i32 @_Z22always_inline_functioni(i32 %a) [[OPTNONE]]
> +
> +int user_of_always_inline_function() {
> + return always_inline_function(4);
> +}
> +
> +// CHECK: define i32 @_Z30user_of_always_inline_functionv() [[NORMAL]]
> +
> +// Test optnone on declaration and always_inline on definition.
> +extern int optnone_function(int a) __attribute__((optnone));
> +
> +__attribute__((always_inline))
> +int optnone_function(int a) {
> + return a + a + a + a;
> +}
> +// CHECK: define i32 @_Z16optnone_functioni(i32 %a) [[OPTNONE]]
> +
> +int user_of_optnone_function() {
> + return optnone_function(4);
> +}
> +
> +// CHECK: define i32 @_Z24user_of_optnone_functionv() [[NORMAL]]
> +
> +// Test the combination of optnone with forceinline (optnone wins).
> +extern __forceinline int forceinline_optnone_function(int a, int b);
> +
> +__attribute__((optnone))
> +extern int forceinline_optnone_function(int a, int b) {
> + return a + b;
> +}
> +
> +int user_of_forceinline_optnone_function() {
> + return forceinline_optnone_function(4,5);
> +}
> +
> +// CHECK: @_Z36user_of_forceinline_optnone_functionv() [[NORMAL]]
> +// CHECK: @_Z28forceinline_optnone_functionii(i32 %a, i32 %b) [[OPTNONE]]
> +
> +// CHECK: attributes [[OPTNONE]] = { noinline nounwind optnone {{.*}} }
> +// CHECK: attributes [[NORMAL]] = { nounwind {{.*}} }
> +
>
More information about the cfe-commits
mailing list