[PATCH] D126984: [clang] Add initial support for gcc's optimize function attribute

[Stephen Long via Phabricator via cfe-commits]

steplong updated this revision to Diff 436551.
steplong added a comment.

- Remove -Og, and all non-zero optimization levels
- Fix up docs
- Modified tests to reflect -Og and non-zero opt level change


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126984/new/

https://reviews.llvm.org/D126984

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/attr-optimize.c
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/attr-optimize.c

Index: clang/test/Sema/attr-optimize.c
===
--- /dev/null
+++ clang/test/Sema/attr-optimize.c
@@ -0,0 +1,52 @@
+// RUN: %clang_cc1 -verify -fsyntax-only %s
+
+__attribute__((optimize(a))) // expected-error {{use of undeclared identifier 'a'}}
+void
+f1() {}
+
+int b = 1;
+__attribute__((optimize(b))) // expected-error {{'optimize' attribute requires a string}}
+void
+f2() {}
+
+__attribute__((optimize("O0", "O1"))) // expected-error {{'optimize' attribute takes one argument}}
+void
+f3() {}
+
+__attribute__((optimize("Og"))) // expected-warning {{invalid optimization level 'Og' specified; only '-O0', '-Os', '-Oz', and '-Ofast' are supported; attribute ignored}}
+void
+f4() {}
+
+__attribute__((optimize("O-1"))) // expected-warning {{invalid optimization level 'O-1' specified; only '-O0', '-Os', '-Oz', and '-Ofast' are supported; attribute ignored}}
+void
+f5() {}
+
+__attribute__((optimize("O+1"))) // expected-warning {{invalid optimization level 'O+1' specified; only '-O0', '-Os', '-Oz', and '-Ofast' are supported; attribute ignored}}
+void
+f6() {}
+
+__attribute__((optimize("O0"))) // expected-no-error
+void
+f7() {}
+
+__attribute__((optimize("Os"))) // expected-no-error
+void
+f8() {}
+
+__attribute__((optimize("O44"))) // expected-warning {{invalid optimization level 'O44' specified; only '-O0', '-Os', '-Oz', and '-Ofast' are supported; attribute ignored}}
+void
+f9() {}
+
+__attribute__((optimize("Oz"))) // expected-no-error
+void
+f10() {}
+
+__attribute__((optimize("Ofast"))) // expected-no-error
+void
+f11() {}
+
+__attribute__((optimize("O"))) // expected-no-error
+void
+f12() {}
+
+__attribute__((optimize("O0"))) // expected-error {{expected identifier or '('}}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -142,6 +142,7 @@
 // CHECK-NEXT: ObjCSubclassingRestricted (SubjectMatchRule_objc_interface)
 // CHECK-NEXT: OpenCLIntelReqdSubGroupSize (SubjectMatchRule_function)
 // CHECK-NEXT: OpenCLNoSVM (SubjectMatchRule_variable)
+// CHECK-NEXT: Optimize (SubjectMatchRule_function)
 // CHECK-NEXT: OptimizeNone (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: Overloadable (SubjectMatchRule_function)
 // CHECK-NEXT: Owner (SubjectMatchRule_record_not_is_union)
Index: clang/test/CodeGen/attr-optimize.c
===
--- /dev/null
+++ clang/test/CodeGen/attr-optimize.c
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -O2 -S -emit-llvm %s -o - | FileCheck %s --check-prefix=O2
+// RUN: %clang_cc1 -O0 -S -emit-llvm %s -o - | FileCheck %s --check-prefix=O0
+
+__attribute__((optimize("O0"))) void f1(void) {}
+// O2: @f1{{.*}}[[ATTR_OPTNONE:#[0-9]+]]
+// O0: @f1{{.*}}[[ATTR_OPTNONE:#[0-9]+]]
+
+__attribute__((optimize("Os"))) void f2(void) {}
+// O2: @f2{{.*}}[[ATTR_OPTSIZE:#[0-9]+]]
+// O0: @f2{{.*}}[[ATTR_OPTNONE]]
+
+__attribute__((optimize("Oz"))) void f4(void) {}
+// O2: @f4{{.*}}[[ATTR_MINSIZE:#[0-9]+]]
+// O0: @f4{{.*}}[[ATTR_OPTNONE]]
+
+__attribute__((optimize("Ofast"))) void f5(void) {}
+// O2: @f5{{.*}}[[ATTR_OPTSIZE]]
+// O0: @f5{{.*}}[[ATTR_OPTNONE]]
+
+// O2: attributes [[ATTR_OPTNONE]] = { {{.*}}optnone{{.*}} }
+// O2: attributes [[ATTR_OPTSIZE]] = { {{.*}}optsize{{.*}} }
+// O2: attributes [[ATTR_MINSIZE]] = { {{.*}}minsize{{.*}}optsize{{.*}} }
+
+// Check that O0 overrides the attribute
+// O0: attributes [[ATTR_OPTNONE]] = { {{.*}}optnone{{.*}} }
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -4833,6 +4833,43 @@
 D->addAttr(Optnone);
 }
 
+static void handleOptimizeAttr(Sema , Decl *D, const ParsedAttr ) {
+  StringRef Arg;
+  if (!S.checkStringLiteralArgumentAttr(AL, 0, Arg))
+return;
+
+  StringRef Level;
+  // Check if argument is prefixed with "-O" or "O"
+  if (Arg.str().rfind("-O", 0) == 0)
+Level = Arg.substr(2);
+  

[PATCH] D126984: [clang] Add initial support for gcc's optimize function attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D126984#3578950 , @steplong wrote:

>>> and even for the MSVC pragma `#pragma optimize("t", on)`, what are we 
>>> supporting if the user compiles their code with `-O0`? because right now we 
>>> won't optimize anything with `-O0`
>>
>> @steplong -- what are your thoughts on this?
>
> Hmm, I think I'm ok with ignoring the pragma when "-O0". In the case of "t", 
> at the moment, we are just going to honor whatever is passed on the 
> commandline. I think with what the patch looks like now, we'll be supporting 
> the pragma optimize like:
>
> | Parameter | On   | Off  
>   |
> | - |  |  
>   |
> | g | Deprecated   | Deprecated   
>   |
> | s | Add OptimizeAttr::Os | Add Optnone (Not sure if this makes 
> sense) |
> | t | Do nothing   | Add Optnone  
>   |
> | y | Do nothing   | Do nothing   
>   |
> |

I think that works for me.




Comment at: clang/docs/ReleaseNotes.rst:331-333
+- Added preliminary support for GCC's attribute ``optimize``, which allows
+  functions to be compiled with different optimization options than what was
+  specified on the command line.

And we can clarify in the release note that we're not intending to fully 
support this attribute.



Comment at: clang/include/clang/Basic/Attr.td:2267-2268
+  EnumArgument<"OptLevel", "OptLevelKind",
+   ["Ofast", "Og", "Oz", "Os", "O0", "O1", "O2", "O3", 
"O4"],
+   ["Ofast", "Og", "Oz", "Os", "O0", "O1", "O2", "O3", 
"O4"],
+   /*optional*/0, /*fake*/1>];

Assuming this also addresses @aeubanks 's concerns, I think we should remove O1 
through O4 and maybe consider renaming the other enumerations to be less about 
the command line option and more about the effects. e.g., `Fast`, `MinSize`, 
`NoOpts` etc. We'll still do the mapping from O0 and whatnot to these values 
(within SemaDeclAttr.cpp) but this should hopefully clarify that the semantics 
we're after are not really pipeline semantics.



Comment at: clang/include/clang/Basic/AttrDocs.td:3462
+  let Content = [{
+The ``optimize`` attribute, when attached to a function, indicates that the
+function should be compiled with a different optimization level than specified

And we can add to the documentation that we don't intend to fully support the 
GCC semantics and further comment about ignoring O1 through O4, etc.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126984/new/

https://reviews.llvm.org/D126984

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126984: [clang] Add initial support for gcc's optimize function attribute

[Stephen Long via Phabricator via cfe-commits]

steplong added a comment.

>> and even for the MSVC pragma `#pragma optimize("t", on)`, what are we 
>> supporting if the user compiles their code with `-O0`? because right now we 
>> won't optimize anything with `-O0`
>
> @steplong -- what are your thoughts on this?

Hmm, I think I'm ok with ignoring the pragma when "-O0". In the case of "t", at 
the moment, we are just going to honor whatever is passed on the commandline. I 
think with what the patch looks like now, we'll be supporting the pragma 
optimize like:

| Parameter | On   | Off
|
| - |  |
|
| g | Deprecated   | Deprecated 
|
| s | Add OptimizeAttr::Os | Add Optnone (Not sure if this makes sense) 
|
| t | Do nothing   | Add Optnone
|
| y | Do nothing   | Do nothing 
|


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126984/new/

https://reviews.llvm.org/D126984

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126984: [clang] Add initial support for gcc's optimize function attribute

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

>> Would that work for you (and you as well @aeubanks)?

yes :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126984/new/

https://reviews.llvm.org/D126984

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126984: [clang] Add initial support for gcc's optimize function attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D126984#3574550 , @aeubanks wrote:

> I can't speak for @xbolva00 but the only part I'm against is the user-visible 
> feature of
>
>> - Added preliminary support for GCC's attribute `optimize`, which allows 
>> functions to be compiled with different optimization options than what was 
>> specified on the command line.
>
> which implies that we're on the way to support per-function optimization 
> levels (which we aren't)
> the internal clang representation changes are all fine

Ah, would you be okay if we retained the user-facing feature but more clearly 
documented (in release notes and documentation) the differences from GCC and 
that we do not currently intend to close that gap?

> and even for the MSVC pragma `#pragma optimize("t", on)`, what are we 
> supporting if the user compiles their code with `-O0`? because right now we 
> won't optimize anything with `-O0`

@steplong -- what are your thoughts on this?

In D126984#3574592 , @xbolva00 wrote:

>> Or are you opposed to the notion of having one semantic attribute to control 
>> all of this and you prefer to see multiple individual semantic attributes 
>> and all that comes along with them in terms of combinations?
>
> But to use different pipeline for different functions (here I mean -O1, O2 
> , O3 
> ) is a major change to LLVM pass 
> manager and I think this use case does not justify it.

Thanks for clarifying! I'd be fine changing the internal enumeration for the 
attribute to represent a better subset of what we intend to implement support 
for (rather than making it look like we intend to support O1 
-O4). Would that work for you (and 
you as well @aeubanks)?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126984/new/

https://reviews.llvm.org/D126984

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126984: [clang] Add initial support for gcc's optimize function attribute

[Stephen Long via Phabricator via cfe-commits]

steplong updated this revision to Diff 436443.
steplong added a comment.

- Add llvm::Attribute::MinSize when OptimizeAttr::Oz
- Add test for checking minsize


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126984/new/

https://reviews.llvm.org/D126984

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/attr-optimize.c
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/attr-optimize.c

Index: clang/test/Sema/attr-optimize.c
===
--- /dev/null
+++ clang/test/Sema/attr-optimize.c
@@ -0,0 +1,52 @@
+// RUN: %clang_cc1 -verify -fsyntax-only %s
+
+__attribute__((optimize(a))) // expected-error {{use of undeclared identifier 'a'}}
+void
+f1() {}
+
+int b = 1;
+__attribute__((optimize(b))) // expected-error {{'optimize' attribute requires a string}}
+void
+f2() {}
+
+__attribute__((optimize("O0", "O1"))) // expected-error {{'optimize' attribute takes one argument}}
+void
+f3() {}
+
+__attribute__((optimize("Og"))) // expected-no-error
+void
+f4() {}
+
+__attribute__((optimize("O-1"))) // expected-warning {{invalid optimization level 'O-1' specified; the argument to '-O' should be a non-negative integer, '-Os', '-Oz', '-Og', or '-Ofast'; attribute ignored}}
+void
+f5() {}
+
+__attribute__((optimize("O+1"))) // expected-warning {{invalid optimization level 'O+1' specified; the argument to '-O' should be a non-negative integer, '-Os', '-Oz', '-Og', or '-Ofast'; attribute ignored}}
+void
+f6() {}
+
+__attribute__((optimize("O0"))) // expected-no-error
+void
+f7() {}
+
+__attribute__((optimize("Os"))) // expected-no-error
+void
+f8() {}
+
+__attribute__((optimize("O44"))) // expected-no-error
+void
+f9() {}
+
+__attribute__((optimize("Oz"))) // expected-no-error
+void
+f10() {}
+
+__attribute__((optimize("Ofast"))) // expected-no-error
+void
+f11() {}
+
+__attribute__((optimize("O"))) // expected-no-error
+void
+f12() {}
+
+__attribute__((optimize("O0"))) // expected-error {{expected identifier or '('}}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -142,6 +142,7 @@
 // CHECK-NEXT: ObjCSubclassingRestricted (SubjectMatchRule_objc_interface)
 // CHECK-NEXT: OpenCLIntelReqdSubGroupSize (SubjectMatchRule_function)
 // CHECK-NEXT: OpenCLNoSVM (SubjectMatchRule_variable)
+// CHECK-NEXT: Optimize (SubjectMatchRule_function)
 // CHECK-NEXT: OptimizeNone (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: Overloadable (SubjectMatchRule_function)
 // CHECK-NEXT: Owner (SubjectMatchRule_record_not_is_union)
Index: clang/test/CodeGen/attr-optimize.c
===
--- /dev/null
+++ clang/test/CodeGen/attr-optimize.c
@@ -0,0 +1,29 @@
+// RUN: %clang_cc1 -O2 -S -emit-llvm %s -o - | FileCheck %s --check-prefix=O2
+// RUN: %clang_cc1 -O0 -S -emit-llvm %s -o - | FileCheck %s --check-prefix=O0
+
+__attribute__((optimize("O0"))) void f1(void) {}
+// O2: @f1{{.*}}[[ATTR_OPTNONE:#[0-9]+]]
+// O0: @f1{{.*}}[[ATTR_OPTNONE:#[0-9]+]]
+
+__attribute__((optimize("Os"))) void f2(void) {}
+// O2: @f2{{.*}}[[ATTR_OPTSIZE:#[0-9]+]]
+// O0: @f2{{.*}}[[ATTR_OPTNONE]]
+
+__attribute__((optimize("Og"))) void f3(void) {}
+// O2: @f3{{.*}}[[ATTR_OPTSIZE]]
+// O0: @f3{{.*}}[[ATTR_OPTNONE]]
+
+__attribute__((optimize("Oz"))) void f4(void) {}
+// O2: @f4{{.*}}[[ATTR_MINSIZE:#[0-9]+]]
+// O0: @f4{{.*}}[[ATTR_OPTNONE]]
+
+__attribute__((optimize("Ofast"))) void f5(void) {}
+// O2: @f5{{.*}}[[ATTR_OPTSIZE]]
+// O0: @f5{{.*}}[[ATTR_OPTNONE]]
+
+// O2: attributes [[ATTR_OPTNONE]] = { {{.*}}optnone{{.*}} }
+// O2: attributes [[ATTR_OPTSIZE]] = { {{.*}}optsize{{.*}} }
+// O2: attributes [[ATTR_MINSIZE]] = { {{.*}}minsize{{.*}}optsize{{.*}} }
+
+// Check that O0 overrides the attribute
+// O0: attributes [[ATTR_OPTNONE]] = { {{.*}}optnone{{.*}} }
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -4833,6 +4833,46 @@
 D->addAttr(Optnone);
 }
 
+static void handleOptimizeAttr(Sema , Decl *D, const ParsedAttr ) {
+  StringRef Arg;
+  if (!S.checkStringLiteralArgumentAttr(AL, 0, Arg))
+return;
+
+  StringRef Level;
+  // Check if argument is prefixed with "-O" or "O"
+  if (Arg.str().rfind("-O", 0) == 0)
+Level = Arg.substr(2);
+  else if (Arg.str().rfind("O", 0) == 0)
+Level = Arg.substr(1);
+  else
+

[PATCH] D126984: [clang] Add initial support for gcc's optimize function attribute

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added inline comments.



Comment at: clang/test/CodeGen/attr-optimize.c:16
+
+__attribute__((optimize("Oz"))) void f4(void) {}
+// O2: @f4{{.*}}[[ATTR_OPTSIZE]]

For -Os, clang adds optsize. For -Oz, clang adds optsize and minsize. So tests 
should check it, maybe currently this is broken in your patch?

https://godbolt.org/z/dEsffoeW4


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126984/new/

https://reviews.llvm.org/D126984

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126984: [clang] Add initial support for gcc's optimize function attribute

[Stephen Long via Phabricator via cfe-commits]

steplong added inline comments.



Comment at: clang/test/CodeGen/attr-optimize.c:4
+
+__attribute__((optimize("O0"))) void f1(void) {}
+// O2: @f1{{.*}}[[ATTR_OPTNONE:#[0-9]+]]

xbolva00 wrote:
> No support for 
> ```
> __attribute__ ((__optimize__ (0)))
> ```
> 
> ? GCC supports it
Nope, I only added support for one argument and only strings. I think gcc 
supports expressions, multiple args, -f args, and -O args. I wasn't sure how to 
implement it in Attr.td without making heavy changes.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126984/new/

https://reviews.llvm.org/D126984

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126984: [clang] Add initial support for gcc's optimize function attribute

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added inline comments.



Comment at: clang/test/CodeGen/attr-optimize.c:4
+
+__attribute__((optimize("O0"))) void f1(void) {}
+// O2: @f1{{.*}}[[ATTR_OPTNONE:#[0-9]+]]

No support for 
```
__attribute__ ((__optimize__ (0)))
```

? GCC supports it


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126984/new/

https://reviews.llvm.org/D126984

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126984: [clang] Add initial support for gcc's optimize function attribute

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

But where I think this feature could be very useful in following case from gcc 
test suite where there is some FP computation..
Imagine you compile you program with -ffast-math and then you have function:

  __attribute__ ((optimize ("no-associative-math"))) double
  fn3 (double h, double l) /* { dg-message "previous definition" } */
  {
return h + l;
  }

So in this case, codegen would just drop llvm attribute "reassoc".


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126984/new/

https://reviews.llvm.org/D126984

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126984: [clang] Add initial support for gcc's optimize function attribute

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

>> Are you opposed to exposing #pragma optimize? 
>> (https://docs.microsoft.com/en-us/cpp/preprocessor/optimize?view=msvc-170) 
>> If yes, I think Stephen should run an RFC on Discourse to see if there's 
>> general agreement.

No, I like it, seems more useful and general than "pragma clang optimize on/off"

>> Are you opposed to the direction of condensing the optimization semantic 
>> attributes (the things in the AST) down into one? If yes, I'd like to 
>> understand why better.

No :)

>> Are you still opposed to exposing a neutered form of the GCC optimize 
>> attribute as a parsed attribute (the thing users write in their source)? If 
>> yes, that's fine by me, but then I'd still like to see most of this patch 
>> land, just without a way for the user to spell the attribute themselves. We 
>> can adjust the semantic attribute's enumeration to cover only the cases we 
>> want to support.

Not entirely opposed, GCC optimize attribute could partially work fine, O0 maps 
to optnone, Os to optsize, Oz to minsize. I am more worried about next steps, 
see below.

>> Or are you opposed to the notion of having one semantic attribute to control 
>> all of this and you prefer to see multiple individual semantic attributes 
>> and all that comes along with them in terms of combinations?

Not strongly opposed, just some concerns how this could work together with 
LLVM. Example: attribute((optimize("-fno-licm"))) -> 'optimize="no-licm" '? 
This could work. Possibly also allow this form: attribute((optimize(no-licm,  
optsize))) ?

What about current attributes? Should/can we drop them and use  for example 
'optimize="no-ipa,no-clone" '? Not strongly opposed, but probably a lot of work.

But to use different pipeline for different functions (here I mean -O1, O2 
, O3 
) is a major change to LLVM pass 
manager and I think this use case does not justify it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126984/new/

https://reviews.llvm.org/D126984

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126984: [clang] Add initial support for gcc's optimize function attribute

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

I can't speak for @xbolva00 but the only part I'm against is the user-visible 
feature of

> - Added preliminary support for GCC's attribute `optimize`, which allows 
> functions to be compiled with different optimization options than what was 
> specified on the command line.

which implies that we're on the way to support per-function optimization levels 
(which we aren't)
the internal clang representation changes are all fine

and even for the MSVC pragma `#pragma optimize("t", on)`, what are we 
supporting if the user compiles their code with `-O0`? because right now we 
won't optimize anything with `-O0`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126984/new/

https://reviews.llvm.org/D126984

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126984: [clang] Add initial support for gcc's optimize function attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D126984#3574445 , @xbolva00 wrote:

> "The optimize attribute should be used for debugging purposes only. It is not 
> suitable in production code."
>
> Until they (users) start and any change in pipeline may surprise them.

Too bad for them? I guess my sympathy button is broken for users who use things 
in production code that are documented as not being suitable for production 
code. :-D

> Personally I am bigger fan of more targeted attributes like we have noinline 
> / noipa proposed but stalled /  and then we could have new ones to disable 
> vectorizers, LICM, unroller, etc..
>
> Yes, we could claim that attribute((optimize("-fno-slp-vectorize") then maps 
> exactly to attribute((noslp)).
>
> Still, I would like to hear some motivation words other than "gcc" has it.

What I want to avoid is the continued proliferation of semantic attributes 
related to optimizations that are otherwise controlled by command line flags. 
We have optnone, minsize, Stephen's original patch for the MSVC pragma added 
another one, you're talking about adding optsize, etc. All of these are 
semantically doing "the same thing", which is associating some coarse 
granularity optimization hints with a function definition that would otherwise 
be even more coarsely controlled via the command line. Having multiple semantic 
attributes makes supporting this more fragile because everywhere that wants to 
care about coarse-grained optimizations has to handle the combinatorial matrix 
of ways they can be mixed together and as that grows, we will invariably get it 
wrong by forgetting something.

What I don't have a strong opinion on is what attributes we surface to users so 
they can spell them in their source. I have no problem exposing GCC's 
attributes, and MSVC's attributes, and our own attributes in whatever fun 
combinations we want to allow. What I want is that all of those related 
attributes are semantically modeled via ONE attribute in the AST. When 
converting these parsed optimization attributes into semantic attributes, I 
want us to map whatever information is in the parsed attribute onto that single 
semantic attribute. When we merge attributes on a declaration, I want that one 
attribute to be updated instead of duplicated with different values. So at the 
end of the day, when we get to CodeGen, we can query for the one attribute and 
its semantic effects instead of querying for numerous attributes and trying to 
decide what to do when more than one attribute is present at that point.

That's why I pushed Stephen to make this patch. The fact that it also happens 
to expose a feature from GCC that is very closely related to what he's trying 
to do for the MSVC pragma was a nice added bonus.

This leaves a few questions:

1. Are you opposed to exposing #pragma optimize? 
(https://docs.microsoft.com/en-us/cpp/preprocessor/optimize?view=msvc-170) If 
yes, I think Stephen should run an RFC on Discourse to see if there's general 
agreement.
2. Are you opposed to the direction of condensing the optimization semantic 
attributes (the things in the AST) down into one? If yes, I'd like to 
understand why better.
3. Are you still opposed to exposing a neutered form of the GCC optimize 
attribute as a parsed attribute (the thing users write in their source)? If 
yes, that's fine by me, but then I'd still like to see most of this patch land, 
just without a way for the user to spell the attribute themselves. We can 
adjust the semantic attribute's enumeration to cover only the cases we want to 
support.
4. Or are you opposed to the notion of having one semantic attribute to control 
all of this and you prefer to see multiple individual semantic attributes and 
all that comes along with them in terms of combinations?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126984/new/

https://reviews.llvm.org/D126984

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126984: [clang] Add initial support for gcc's optimize function attribute

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

"The optimize attribute should be used for debugging purposes only. It is not 
suitable in production code."

Until they (users) start and any change in pipeline may surprise them.

Personally I am bigger fan of more targeted attributes like we have noinline / 
noipa proposed but stalled /  and then we could have new ones to disable 
vectorizers, LICM, unroller, etc..

Yes, we could claim that attribute((optimize("-fno-slp-vectorize") then maps 
exactly to attribute((noslp)).

Still, I would like to hear some motivation words other than "gcc" has it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126984/new/

https://reviews.llvm.org/D126984

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126984: [clang] Add initial support for gcc's optimize function attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D126984#3574288 , @xbolva00 wrote:

> [[gnu::optimize("O0")]] is okay but [[gnu::optimize("O3 
> ")]] is not gonna work without 
> major changes. Not sure why we should deliver half-broken new attribute.

I don't see it as half-broken given that it's explicitly documented by GCC as 
"The optimize attribute should be used for debugging purposes only. It is not 
suitable in production code."

> Some strong motivation/use cases why somebody needs to compile some functions 
> with -O2 and some with -O3?

This patch leaves O1 -O4 as noops, 
and really only does anything with O0 and the letter-based ones, so I don't 
know that we need to do that exercise until we want to make them actually do 
something. Similarly, we don't allow any of the `-f` flags like GCC does.

I don't insist on keeping this attribute specifically, but I like the fact that 
it gives us *one* attribute that we can use as a basis for all these various 
other ways of spelling optimization hints (optnone, #pragma optimize, minsize, 
etc). I think I'd ultimately like to see all of those other attributes as 
alternate spellings of this one and they just map to the appropriate internal 
"level".

(I'm totally fine with us not supporting optimization hints that the backend 
would struggle with, this is more about how we model the notion of a 
per-function user-provided optimization hint without adding a new attribute 
every time.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126984/new/

https://reviews.llvm.org/D126984

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126984: [clang] Add initial support for gcc's optimize function attribute

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

In D126984#3574071 , @xbolva00 wrote:

> I agree with @aeubanks , this feature requires major changes to pass manager 
> and I see no value to land this currently.
>
> I see that somebody may prefer “opt for size”, but this is exposed via 
> “minsize” attribute so I see no strong need for optimize(“-Os”)

Hmm.. We expose minsize attribute in C, (like -Oz), but "optsize" is not 
exposed (like -Os). I will try to create a patch for exposing it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126984/new/

https://reviews.llvm.org/D126984

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126984: [clang] Add initial support for gcc's optimize function attribute

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

[[gnu::optimize("O0")]] is okay but [[gnu::optimize("O3 
")]] is not gonna work without 
major changes. Not sure why we should deliver half-broken new attribute.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126984/new/

https://reviews.llvm.org/D126984

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126984: [clang] Add initial support for gcc's optimize function attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D126984#3574077 , @aeubanks wrote:

> In D126984#3574046 , @aaron.ballman 
> wrote:
>
>> In D126984#3571573 , @aeubanks 
>> wrote:
>>
>>> IIRC in the past there was a strong preference to not have the pass manager 
>>> support this sort of thing
>>> if you want to support this, there should be an RFC for how the 
>>> optimization part of this will work as it may require invasive changes to 
>>> the LLVM pass manager
>>>
>>> (if this is purely a clang frontend thing then ignore me)
>>
>> Hmm, does the pass manager have to support anything here? The only Clang 
>> codegen changes are for emitting IR attributes that we already emitted based 
>> on command line flags/other attributes, so I had the impression this would 
>> not be invasive for the backend at all.
>
> if we're allowing individual functions to specify that they want the `-O1` 
> pipeline when everything else in the module should be compiled with `-O2`, 
> that's a huge change in the pass manager. but perhaps I'm misunderstanding 
> the point of this patch

I guess I'm not seeing what burden is being added here (you may still be 
correct though!)

The codegen changes basically boil down to:

  if (!HasOptnone) {
if (CodeGenOpts.OptimizeSize || HasOptsize) // This was updated for || 
HasOptsize
  FuncAttrs.addAttribute(llvm::Attribute::OptimizeForSize);
if (CodeGenOpts.OptimizeSize == 2)
  FuncAttrs.addAttribute(llvm::Attribute::MinSize);
  }

and

  bool HasOptimizeAttrO0 = false; // NEW
  if (const auto *OA = D->getAttr())
HasOptimizeAttrO0 = OA->getOptLevel() == OptimizeAttr::O0;
  
  // Add optnone, but do so only if the function isn't always_inline.
  if ((ShouldAddOptNone || D->hasAttr() ||
   HasOptimizeAttrO0) &&  // NEW
  !F->hasFnAttribute(llvm::Attribute::AlwaysInline)) {
B.addAttribute(llvm::Attribute::OptimizeNone);

For my own education, are you saying that these changes are specifying the 
entire -O pipeline?

The patch looks for `O0` in the attribute, but the other O values are noops. 
We do some mapping though:

  OptimizeAttr::OptLevelKind Kind = OA->getOptLevel();
  HasOptnone = HasOptnone || (Kind == OptimizeAttr::O0);
  HasOptsize = Kind == OptimizeAttr::Os || Kind == OptimizeAttr::Og ||
   Kind == OptimizeAttr::Ofast || Kind == OptimizeAttr::Oz;

but I'm failing to understand why this would be a concern for the backend given 
that we already support setting the LLVM IR flags based on other attributes. 
e.g., why is `[[clang::optnone]]` okay but `[[gnu::optimize("O0")]]` a problem?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126984/new/

https://reviews.llvm.org/D126984

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126984: [clang] Add initial support for gcc's optimize function attribute

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

In D126984#3574091 , @steplong wrote:

> In D126984#3574077 , @aeubanks 
> wrote:
>
>> In D126984#3574046 , 
>> @aaron.ballman wrote:
>>
>>> In D126984#3571573 , @aeubanks 
>>> wrote:
>>>
 IIRC in the past there was a strong preference to not have the pass 
 manager support this sort of thing
 if you want to support this, there should be an RFC for how the 
 optimization part of this will work as it may require invasive changes to 
 the LLVM pass manager

 (if this is purely a clang frontend thing then ignore me)
>>>
>>> Hmm, does the pass manager have to support anything here? The only Clang 
>>> codegen changes are for emitting IR attributes that we already emitted 
>>> based on command line flags/other attributes, so I had the impression this 
>>> would not be invasive for the backend at all.
>>
>> if we're allowing individual functions to specify that they want the `-O1` 
>> pipeline when everything else in the module should be compiled with `-O2`, 
>> that's a huge change in the pass manager. but perhaps I'm misunderstanding 
>> the point of this patch
>
> That makes sense. The MSVC pragma allows 4 options, "stgy":
>
> | Parameter | On  
> | Off 
>|
> | - | 
> ---
>  |
> |
> | g | Deprecated  
> | Deprecated  
>|
> | s | Add MinSize 
> | Remove MinSize (I think this 
> would be difficult to do if -Os is passed on the cmdline) |
> | t | Add -O2 (We can't support -O2 with this attribute so ignore)
> | Add Optnone 
>|
> | y | Add frame-pointers (We can't support -f arguments with the 
> attribute in this patch so we are ignoring this) | No frame-pointers (Same 
> thing as on)   |
> |
>
> For our use case, I think we only really see `#pragma optimize("", off)` and 
> `#pragma optimize("", on)`, so I'm not opposed to abandoning this patch and 
> just supporting the common use case for now. I think `#pragma optimize("", 
> on)` would just mean do nothing and apply whatever is on the command line and 
> `#pragma optimize("", off)` would mean add optnone to the functions below it

looks good to me, I agree that we should just honor whatever optimization level 
the file is compiled with with `t`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126984/new/

https://reviews.llvm.org/D126984

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126984: [clang] Add initial support for gcc's optimize function attribute

[Stephen Long via Phabricator via cfe-commits]

steplong added a comment.

In D126984#3574077 , @aeubanks wrote:

> In D126984#3574046 , @aaron.ballman 
> wrote:
>
>> In D126984#3571573 , @aeubanks 
>> wrote:
>>
>>> IIRC in the past there was a strong preference to not have the pass manager 
>>> support this sort of thing
>>> if you want to support this, there should be an RFC for how the 
>>> optimization part of this will work as it may require invasive changes to 
>>> the LLVM pass manager
>>>
>>> (if this is purely a clang frontend thing then ignore me)
>>
>> Hmm, does the pass manager have to support anything here? The only Clang 
>> codegen changes are for emitting IR attributes that we already emitted based 
>> on command line flags/other attributes, so I had the impression this would 
>> not be invasive for the backend at all.
>
> if we're allowing individual functions to specify that they want the `-O1` 
> pipeline when everything else in the module should be compiled with `-O2`, 
> that's a huge change in the pass manager. but perhaps I'm misunderstanding 
> the point of this patch

That makes sense. The MSVC pragma allows 4 options, "stgy":

| Parameter | On
  | Off 
   |
| - | 
---
 |  
  |
| g | Deprecated
  | Deprecated  
   |
| s | Add MinSize   
  | Remove MinSize (I think this would 
be difficult to do if -Os is passed on the cmdline) |
| t | Add -O2 (We can't support -O2 with this attribute so ignore)  
  | Add Optnone 
   |
| y | Add frame-pointers (We can't support -f arguments with the 
attribute in this patch so we are ignoring this) | No frame-pointers (Same 
thing as on)   |
|

For our use case, I think we only really see `#pragma optimize("", off)` and 
`#pragma optimize("", on)`, so I'm not opposed to abandoning this patch and 
just supporting the common use case for now. I think `#pragma optimize("", on)` 
would just mean do nothing and apply whatever is on the command line and 
`#pragma optimize("", off)` would mean add optnone to the functions below it


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126984/new/

https://reviews.llvm.org/D126984

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126984: [clang] Add initial support for gcc's optimize function attribute

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

In D126984#3574046 , @aaron.ballman 
wrote:

> In D126984#3571573 , @aeubanks 
> wrote:
>
>> IIRC in the past there was a strong preference to not have the pass manager 
>> support this sort of thing
>> if you want to support this, there should be an RFC for how the optimization 
>> part of this will work as it may require invasive changes to the LLVM pass 
>> manager
>>
>> (if this is purely a clang frontend thing then ignore me)
>
> Hmm, does the pass manager have to support anything here? The only Clang 
> codegen changes are for emitting IR attributes that we already emitted based 
> on command line flags/other attributes, so I had the impression this would 
> not be invasive for the backend at all.

if we're allowing individual functions to specify that they want the `-O1` 
pipeline when everything else in the module should be compiled with `-O2`, 
that's a huge change in the pass manager. but perhaps I'm misunderstanding the 
point of this patch


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126984/new/

https://reviews.llvm.org/D126984

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126984: [clang] Add initial support for gcc's optimize function attribute

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

I agree with @aeubanks , this feature requires major changes to pass manager 
and I see no value to land this currently.

I see that somebody may prefer “opt for size”, but this is exposed via 
“minsize” attribute so I see no strong need for optimize(“-Os”)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126984/new/

https://reviews.llvm.org/D126984

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126984: [clang] Add initial support for gcc's optimize function attribute

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

In D126984#3574033 , @steplong wrote:

> In D126984#3571573 , @aeubanks 
> wrote:
>
>> IIRC in the past there was a strong preference to not have the pass manager 
>> support this sort of thing
>> if you want to support this, there should be an RFC for how the optimization 
>> part of this will work as it may require invasive changes to the LLVM pass 
>> manager
>>
>> (if this is purely a clang frontend thing then ignore me)
>
> Hmm, this does affect codegen, so I'm not sure if it's purely a clang 
> frontend thing. Maybe someone else can confirm. The motivation behind this 
> was to add support for MSVC's `pragma optimize` in D125723 
> . 
> https://docs.microsoft.com/en-us/cpp/preprocessor/optimize?view=msvc-170

adding `optsize`/`minsize`/`optnone` attributes to functions is fine and is 
already handled in optimizations, but being able to specify `-O[0-3]` would 
require a lot of new complexity in the pass manager which would likely not be 
worth it
I think D125723  is fine as is


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126984/new/

https://reviews.llvm.org/D126984

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126984: [clang] Add initial support for gcc's optimize function attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D126984#3571573 , @aeubanks wrote:

> IIRC in the past there was a strong preference to not have the pass manager 
> support this sort of thing
> if you want to support this, there should be an RFC for how the optimization 
> part of this will work as it may require invasive changes to the LLVM pass 
> manager
>
> (if this is purely a clang frontend thing then ignore me)

Hmm, does the pass manager have to support anything here? The only Clang 
codegen changes are for emitting IR attributes that we already emitted based on 
command line flags/other attributes, so I had the impression this would not be 
invasive for the backend at all.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126984/new/

https://reviews.llvm.org/D126984

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126984: [clang] Add initial support for gcc's optimize function attribute

[Stephen Long via Phabricator via cfe-commits]

steplong added a comment.

In D126984#3571573 , @aeubanks wrote:

> IIRC in the past there was a strong preference to not have the pass manager 
> support this sort of thing
> if you want to support this, there should be an RFC for how the optimization 
> part of this will work as it may require invasive changes to the LLVM pass 
> manager
>
> (if this is purely a clang frontend thing then ignore me)

Hmm, this does affect codegen, so I'm not sure if it's purely a clang frontend 
thing. Maybe someone else can confirm. The motivation behind this was to add 
support for MSVC's `pragma optimize` in D125723 
. 
https://docs.microsoft.com/en-us/cpp/preprocessor/optimize?view=msvc-170


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126984/new/

https://reviews.llvm.org/D126984

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126984: [clang] Add initial support for gcc's optimize function attribute

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks requested changes to this revision.
aeubanks added a comment.
This revision now requires changes to proceed.

IIRC in the past there was a strong preference to not have the pass manager 
support this sort of thing
if you want to support this, there should be an RFC for how the optimization 
part of this will work as it may require invasive changes to the LLVM pass 
manager

(if this is purely a clang frontend thing then ignore me)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126984/new/

https://reviews.llvm.org/D126984

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126984: [clang] Add initial support for gcc's optimize function attribute

[Stephen Long via Phabricator via cfe-commits]

steplong updated this revision to Diff 435680.
steplong added a comment.

- Added release notes on attribute


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126984/new/

https://reviews.llvm.org/D126984

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/attr-optimize.c
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/attr-optimize.c

Index: clang/test/Sema/attr-optimize.c
===
--- /dev/null
+++ clang/test/Sema/attr-optimize.c
@@ -0,0 +1,52 @@
+// RUN: %clang_cc1 -verify -fsyntax-only %s
+
+__attribute__((optimize(a))) // expected-error {{use of undeclared identifier 'a'}}
+void
+f1() {}
+
+int b = 1;
+__attribute__((optimize(b))) // expected-error {{'optimize' attribute requires a string}}
+void
+f2() {}
+
+__attribute__((optimize("O0", "O1"))) // expected-error {{'optimize' attribute takes one argument}}
+void
+f3() {}
+
+__attribute__((optimize("Og"))) // expected-no-error
+void
+f4() {}
+
+__attribute__((optimize("O-1"))) // expected-warning {{invalid optimization level 'O-1' specified; the argument to '-O' should be a non-negative integer, '-Os', '-Oz', '-Og', or '-Ofast'; attribute ignored}}
+void
+f5() {}
+
+__attribute__((optimize("O+1"))) // expected-warning {{invalid optimization level 'O+1' specified; the argument to '-O' should be a non-negative integer, '-Os', '-Oz', '-Og', or '-Ofast'; attribute ignored}}
+void
+f6() {}
+
+__attribute__((optimize("O0"))) // expected-no-error
+void
+f7() {}
+
+__attribute__((optimize("Os"))) // expected-no-error
+void
+f8() {}
+
+__attribute__((optimize("O44"))) // expected-no-error
+void
+f9() {}
+
+__attribute__((optimize("Oz"))) // expected-no-error
+void
+f10() {}
+
+__attribute__((optimize("Ofast"))) // expected-no-error
+void
+f11() {}
+
+__attribute__((optimize("O"))) // expected-no-error
+void
+f12() {}
+
+__attribute__((optimize("O0"))) // expected-error {{expected identifier or '('}}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -142,6 +142,7 @@
 // CHECK-NEXT: ObjCSubclassingRestricted (SubjectMatchRule_objc_interface)
 // CHECK-NEXT: OpenCLIntelReqdSubGroupSize (SubjectMatchRule_function)
 // CHECK-NEXT: OpenCLNoSVM (SubjectMatchRule_variable)
+// CHECK-NEXT: Optimize (SubjectMatchRule_function)
 // CHECK-NEXT: OptimizeNone (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: Overloadable (SubjectMatchRule_function)
 // CHECK-NEXT: Owner (SubjectMatchRule_record_not_is_union)
Index: clang/test/CodeGen/attr-optimize.c
===
--- /dev/null
+++ clang/test/CodeGen/attr-optimize.c
@@ -0,0 +1,28 @@
+// RUN: %clang_cc1 -O2 -S -emit-llvm %s -o - | FileCheck %s --check-prefix=O2
+// RUN: %clang_cc1 -O0 -S -emit-llvm %s -o - | FileCheck %s --check-prefix=O0
+
+__attribute__((optimize("O0"))) void f1(void) {}
+// O2: @f1{{.*}}[[ATTR_OPTNONE:#[0-9]+]]
+// O0: @f1{{.*}}[[ATTR_OPTNONE:#[0-9]+]]
+
+__attribute__((optimize("Os"))) void f2(void) {}
+// O2: @f2{{.*}}[[ATTR_OPTSIZE:#[0-9]+]]
+// O0: @f2{{.*}}[[ATTR_OPTNONE]]
+
+__attribute__((optimize("Og"))) void f3(void) {}
+// O2: @f3{{.*}}[[ATTR_OPTSIZE]]
+// O0: @f3{{.*}}[[ATTR_OPTNONE]]
+
+__attribute__((optimize("Oz"))) void f4(void) {}
+// O2: @f4{{.*}}[[ATTR_OPTSIZE]]
+// O0: @f4{{.*}}[[ATTR_OPTNONE]]
+
+__attribute__((optimize("Ofast"))) void f5(void) {}
+// O2: @f5{{.*}}[[ATTR_OPTSIZE]]
+// O0: @f5{{.*}}[[ATTR_OPTNONE]]
+
+// O2: attributes [[ATTR_OPTNONE]] = { {{.*}}optnone{{.*}} }
+// O2: attributes [[ATTR_OPTSIZE]] = { {{.*}}optsize{{.*}} }
+
+// Check that O0 overrides the attribute
+// O0: attributes [[ATTR_OPTNONE]] = { {{.*}}optnone{{.*}} }
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -4833,6 +4833,46 @@
 D->addAttr(Optnone);
 }
 
+static void handleOptimizeAttr(Sema , Decl *D, const ParsedAttr ) {
+  StringRef Arg;
+  if (!S.checkStringLiteralArgumentAttr(AL, 0, Arg))
+return;
+
+  StringRef Level;
+  // Check if argument is prefixed with "-O" or "O"
+  if (Arg.str().rfind("-O", 0) == 0)
+Level = Arg.substr(2);
+  else if (Arg.str().rfind("O", 0) == 0)
+Level = Arg.substr(1);
+  else
+S.Diag(AL.getLoc(), diag::warn_invalid_optimize_attr_level) << Arg;
+
+  llvm::StringMap StrToKind = {
+  {"", OptimizeAttr::O0},  {"s", 

[PATCH] D126984: [clang] Add initial support for gcc's optimize function attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM, but please add the release note when landing. Thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126984/new/

https://reviews.llvm.org/D126984

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126984: [clang] Add initial support for gcc's optimize function attribute

[Stephen Long via Phabricator via cfe-commits]

steplong added a comment.

In D126984#3567822 , @aaron.ballman 
wrote:

> Two final (I hope) nits! One is in the code, the other is that this should 
> have a release note for the new attribute. Assuming precommit CI pipeline 
> passes, then I think this is all set.

Haha, no worries! I appreciate the review


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126984/new/

https://reviews.llvm.org/D126984

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126984: [clang] Add initial support for gcc's optimize function attribute

[Stephen Long via Phabricator via cfe-commits]

steplong updated this revision to Diff 435313.
steplong added a comment.

- Add Arg when creating Attr


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126984/new/

https://reviews.llvm.org/D126984

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/attr-optimize.c
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/attr-optimize.c

Index: clang/test/Sema/attr-optimize.c
===
--- /dev/null
+++ clang/test/Sema/attr-optimize.c
@@ -0,0 +1,52 @@
+// RUN: %clang_cc1 -verify -fsyntax-only %s
+
+__attribute__((optimize(a))) // expected-error {{use of undeclared identifier 'a'}}
+void
+f1() {}
+
+int b = 1;
+__attribute__((optimize(b))) // expected-error {{'optimize' attribute requires a string}}
+void
+f2() {}
+
+__attribute__((optimize("O0", "O1"))) // expected-error {{'optimize' attribute takes one argument}}
+void
+f3() {}
+
+__attribute__((optimize("Og"))) // expected-no-error
+void
+f4() {}
+
+__attribute__((optimize("O-1"))) // expected-warning {{invalid optimization level 'O-1' specified; the argument to '-O' should be a non-negative integer, '-Os', '-Oz', '-Og', or '-Ofast'; attribute ignored}}
+void
+f5() {}
+
+__attribute__((optimize("O+1"))) // expected-warning {{invalid optimization level 'O+1' specified; the argument to '-O' should be a non-negative integer, '-Os', '-Oz', '-Og', or '-Ofast'; attribute ignored}}
+void
+f6() {}
+
+__attribute__((optimize("O0"))) // expected-no-error
+void
+f7() {}
+
+__attribute__((optimize("Os"))) // expected-no-error
+void
+f8() {}
+
+__attribute__((optimize("O44"))) // expected-no-error
+void
+f9() {}
+
+__attribute__((optimize("Oz"))) // expected-no-error
+void
+f10() {}
+
+__attribute__((optimize("Ofast"))) // expected-no-error
+void
+f11() {}
+
+__attribute__((optimize("O"))) // expected-no-error
+void
+f12() {}
+
+__attribute__((optimize("O0"))) // expected-error {{expected identifier or '('}}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -142,6 +142,7 @@
 // CHECK-NEXT: ObjCSubclassingRestricted (SubjectMatchRule_objc_interface)
 // CHECK-NEXT: OpenCLIntelReqdSubGroupSize (SubjectMatchRule_function)
 // CHECK-NEXT: OpenCLNoSVM (SubjectMatchRule_variable)
+// CHECK-NEXT: Optimize (SubjectMatchRule_function)
 // CHECK-NEXT: OptimizeNone (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: Overloadable (SubjectMatchRule_function)
 // CHECK-NEXT: Owner (SubjectMatchRule_record_not_is_union)
Index: clang/test/CodeGen/attr-optimize.c
===
--- /dev/null
+++ clang/test/CodeGen/attr-optimize.c
@@ -0,0 +1,28 @@
+// RUN: %clang_cc1 -O2 -S -emit-llvm %s -o - | FileCheck %s --check-prefix=O2
+// RUN: %clang_cc1 -O0 -S -emit-llvm %s -o - | FileCheck %s --check-prefix=O0
+
+__attribute__((optimize("O0"))) void f1(void) {}
+// O2: @f1{{.*}}[[ATTR_OPTNONE:#[0-9]+]]
+// O0: @f1{{.*}}[[ATTR_OPTNONE:#[0-9]+]]
+
+__attribute__((optimize("Os"))) void f2(void) {}
+// O2: @f2{{.*}}[[ATTR_OPTSIZE:#[0-9]+]]
+// O0: @f2{{.*}}[[ATTR_OPTNONE]]
+
+__attribute__((optimize("Og"))) void f3(void) {}
+// O2: @f3{{.*}}[[ATTR_OPTSIZE]]
+// O0: @f3{{.*}}[[ATTR_OPTNONE]]
+
+__attribute__((optimize("Oz"))) void f4(void) {}
+// O2: @f4{{.*}}[[ATTR_OPTSIZE]]
+// O0: @f4{{.*}}[[ATTR_OPTNONE]]
+
+__attribute__((optimize("Ofast"))) void f5(void) {}
+// O2: @f5{{.*}}[[ATTR_OPTSIZE]]
+// O0: @f5{{.*}}[[ATTR_OPTNONE]]
+
+// O2: attributes [[ATTR_OPTNONE]] = { {{.*}}optnone{{.*}} }
+// O2: attributes [[ATTR_OPTSIZE]] = { {{.*}}optsize{{.*}} }
+
+// Check that O0 overrides the attribute
+// O0: attributes [[ATTR_OPTNONE]] = { {{.*}}optnone{{.*}} }
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -4833,6 +4833,46 @@
 D->addAttr(Optnone);
 }
 
+static void handleOptimizeAttr(Sema , Decl *D, const ParsedAttr ) {
+  StringRef Arg;
+  if (!S.checkStringLiteralArgumentAttr(AL, 0, Arg))
+return;
+
+  StringRef Level;
+  // Check if argument is prefixed with "-O" or "O"
+  if (Arg.str().rfind("-O", 0) == 0)
+Level = Arg.substr(2);
+  else if (Arg.str().rfind("O", 0) == 0)
+Level = Arg.substr(1);
+  else
+S.Diag(AL.getLoc(), diag::warn_invalid_optimize_attr_level) << Arg;
+
+  llvm::StringMap StrToKind = {
+  {"", OptimizeAttr::O0},  {"s", OptimizeAttr::Os},
+  {"g", 

[PATCH] D126984: [clang] Add initial support for gcc's optimize function attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Two final (I hope) nits! One is in the code, the other is that this should have 
a release note for the new attribute. Assuming precommit CI pipeline passes, 
then I think this is all set.




Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4860
+  if (It != StrToKind.end()) {
+D->addAttr(::new (S.Context) OptimizeAttr(S.Context, AL, "", It->second));
+return;

Sorry for missing this before, but on both of the calls to `Create` here, you 
should be passing in `Arg` so that the AST retains full source fidelity (this 
matters for things like pretty printing).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126984/new/

https://reviews.llvm.org/D126984

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126984: [clang] Add initial support for gcc's optimize function attribute

[Stephen Long via Phabricator via cfe-commits]

steplong updated this revision to Diff 435264.
steplong added a comment.
Herald added a subscriber: jdoerfert.

- Fix up docs and comments
- Fix failing pragma-attribute-supported-attributes-list.test
- Remove debug print
- Change to StringMap


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126984/new/

https://reviews.llvm.org/D126984

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/attr-optimize.c
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/attr-optimize.c

Index: clang/test/Sema/attr-optimize.c
===
--- /dev/null
+++ clang/test/Sema/attr-optimize.c
@@ -0,0 +1,52 @@
+// RUN: %clang_cc1 -verify -fsyntax-only %s
+
+__attribute__((optimize(a))) // expected-error {{use of undeclared identifier 'a'}}
+void
+f1() {}
+
+int b = 1;
+__attribute__((optimize(b))) // expected-error {{'optimize' attribute requires a string}}
+void
+f2() {}
+
+__attribute__((optimize("O0", "O1"))) // expected-error {{'optimize' attribute takes one argument}}
+void
+f3() {}
+
+__attribute__((optimize("Og"))) // expected-no-error
+void
+f4() {}
+
+__attribute__((optimize("O-1"))) // expected-warning {{invalid optimization level 'O-1' specified; the argument to '-O' should be a non-negative integer, '-Os', '-Oz', '-Og', or '-Ofast'; attribute ignored}}
+void
+f5() {}
+
+__attribute__((optimize("O+1"))) // expected-warning {{invalid optimization level 'O+1' specified; the argument to '-O' should be a non-negative integer, '-Os', '-Oz', '-Og', or '-Ofast'; attribute ignored}}
+void
+f6() {}
+
+__attribute__((optimize("O0"))) // expected-no-error
+void
+f7() {}
+
+__attribute__((optimize("Os"))) // expected-no-error
+void
+f8() {}
+
+__attribute__((optimize("O44"))) // expected-no-error
+void
+f9() {}
+
+__attribute__((optimize("Oz"))) // expected-no-error
+void
+f10() {}
+
+__attribute__((optimize("Ofast"))) // expected-no-error
+void
+f11() {}
+
+__attribute__((optimize("O"))) // expected-no-error
+void
+f12() {}
+
+__attribute__((optimize("O0"))) // expected-error {{expected identifier or '('}}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -142,6 +142,7 @@
 // CHECK-NEXT: ObjCSubclassingRestricted (SubjectMatchRule_objc_interface)
 // CHECK-NEXT: OpenCLIntelReqdSubGroupSize (SubjectMatchRule_function)
 // CHECK-NEXT: OpenCLNoSVM (SubjectMatchRule_variable)
+// CHECK-NEXT: Optimize (SubjectMatchRule_function)
 // CHECK-NEXT: OptimizeNone (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: Overloadable (SubjectMatchRule_function)
 // CHECK-NEXT: Owner (SubjectMatchRule_record_not_is_union)
Index: clang/test/CodeGen/attr-optimize.c
===
--- /dev/null
+++ clang/test/CodeGen/attr-optimize.c
@@ -0,0 +1,28 @@
+// RUN: %clang_cc1 -O2 -S -emit-llvm %s -o - | FileCheck %s --check-prefix=O2
+// RUN: %clang_cc1 -O0 -S -emit-llvm %s -o - | FileCheck %s --check-prefix=O0
+
+__attribute__((optimize("O0"))) void f1(void) {}
+// O2: @f1{{.*}}[[ATTR_OPTNONE:#[0-9]+]]
+// O0: @f1{{.*}}[[ATTR_OPTNONE:#[0-9]+]]
+
+__attribute__((optimize("Os"))) void f2(void) {}
+// O2: @f2{{.*}}[[ATTR_OPTSIZE:#[0-9]+]]
+// O0: @f2{{.*}}[[ATTR_OPTNONE]]
+
+__attribute__((optimize("Og"))) void f3(void) {}
+// O2: @f3{{.*}}[[ATTR_OPTSIZE]]
+// O0: @f3{{.*}}[[ATTR_OPTNONE]]
+
+__attribute__((optimize("Oz"))) void f4(void) {}
+// O2: @f4{{.*}}[[ATTR_OPTSIZE]]
+// O0: @f4{{.*}}[[ATTR_OPTNONE]]
+
+__attribute__((optimize("Ofast"))) void f5(void) {}
+// O2: @f5{{.*}}[[ATTR_OPTSIZE]]
+// O0: @f5{{.*}}[[ATTR_OPTNONE]]
+
+// O2: attributes [[ATTR_OPTNONE]] = { {{.*}}optnone{{.*}} }
+// O2: attributes [[ATTR_OPTSIZE]] = { {{.*}}optsize{{.*}} }
+
+// Check that O0 overrides the attribute
+// O0: attributes [[ATTR_OPTNONE]] = { {{.*}}optnone{{.*}} }
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -4833,6 +4833,46 @@
 D->addAttr(Optnone);
 }
 
+static void handleOptimizeAttr(Sema , Decl *D, const ParsedAttr ) {
+  StringRef Arg;
+  if (!S.checkStringLiteralArgumentAttr(AL, 0, Arg))
+return;
+
+  StringRef Level;
+  // Check if argument is prefixed with "-O" or "O"
+  if (Arg.str().rfind("-O", 0) == 0)
+Level = Arg.substr(2);
+  else if (Arg.str().rfind("O", 0) == 0)
+Level = Arg.substr(1);
+  else
+S.Diag(AL.getLoc(), 

[PATCH] D126984: [clang] Add initial support for gcc's optimize function attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

I think this is getting close -- mostly just nits at this point.




Comment at: clang/include/clang/Basic/AttrDocs.td:3463
+The ``optimize`` attribute, when attached to a function, indicates that the
+function be compiled with a different optimization level than specified on the
+command line. See the Function Attributes documentation on GCC's docs for more





Comment at: clang/include/clang/Basic/AttrDocs.td:3465-3467
+information. Currently, the attribute differs from GCC's in that we only 
support
+one argument and we don't support "-f" arguments. We also don't support
+expressions and integers as arguments, unlike GCC.





Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1932-1934
+  if (const auto *OA = D->getAttr()) {
+HasOptimizeAttrO0 = OA->getOptLevel() == OptimizeAttr::O0;
+  }

Coding style nit.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4851
+
+  std::unordered_map StrToKind = {
+  {"", OptimizeAttr::O0},  {"s", OptimizeAttr::Os},

Given that most of the strings we're dealing with are either literals or a 
`StringRef`, I think you should use `llvm:: StringMap` instead of 
`unordered_map`.  (We tend to avoid STL containers because their performance 
characteristics are often different than what we need as a compiler.)



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4859
+
+  DEBUG_WITH_TYPE("foo", llvm::dbgs() << "Level: " << Level << "\n");
+

Probably meant to remove this?



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4869
+  if (!Level.getAsInteger(10, Num)) {
+// Limit level to -O4 if higher
+std::string Level = std::to_string(Num.getLimitedValue(4));





Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4841-4850
+  StringRef Level;
+  // Check if argument is prefixed with "-O" or "O"
+  if (Arg.str().rfind("-O", 0) == 0)
+Level = Arg.substr(2);
+  else if (Arg.str().rfind("O", 0) == 0)
+Level = Arg.substr(1);
+  else

steplong wrote:
> aaron.ballman wrote:
> > Then, in here, you can parse the `-O` the user passed as a 
> > string, and convert it to an `OptimizeAttr::OptLevelKind` enumerator and 
> > store that in the semantic attribute.
> > 
> > This allows you to map things like `-Og` to whatever -O level that actually 
> > represents, or do any other kind of mapping that works for you.
> > 
> > One question we should probably figure out is whether we also want to 
> > support clang-cl optimization strings or not. e.g., 
> > `__attribute__((optimize("/O0")))` with a slash instead of a dash. Since 
> > we're already going to be doing parsing from here anyway, I feel like it 
> > might not be a bad idea to also support those. FWIW, here's the list from 
> > the user's manual:
> > ```
> >   /O0 Disable optimization
> >   /O1 Optimize for size  (same as /Og /Os /Oy /Ob2 
> > /GF /Gy)
> >   /O2 Optimize for speed (same as /Og /Oi /Ot /Oy /Ob2 
> > /GF /Gy)
> >   /Ob0Disable function inlining
> >   /Ob1Only inline functions which are (explicitly or 
> > implicitly) marked inline
> >   /Ob2Inline functions as deemed beneficial by the 
> > compiler
> >   /Od Disable optimization
> >   /Og No effect
> >   /Oi-Disable use of builtin functions
> >   /Oi Enable use of builtin functions
> >   /Os Optimize for size
> >   /Ot Optimize for speed
> >   /Ox Deprecated (same as /Og /Oi /Ot /Oy /Ob2); use 
> > /O2 instead
> >   /Oy-Disable frame pointer omission (x86 only, default)
> >   /Oy Enable frame pointer omission (x86 only)
> >   /O   Set multiple /O flags at once; e.g. '/O2y-' for 
> > '/O2 /Oy-'
> > ```
> > (Not all of these would be supported, like enable use of builtin functions, 
> > etc.) WDYT?
> Hmm I don't think it's necessary to get pragma optimize to work, but it 
> shouldn't be hard to add the parsing logic for some of these strings
Definitely agreed on the MSVC command line switches, so if those are a burden, 
feel free to skip them. For the other flags, those seem more important to 
support because they're also flags present in GCC so users are more likely to 
expect them to work.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126984/new/

https://reviews.llvm.org/D126984

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126984: [clang] Add initial support for gcc's optimize function attribute

[Stephen Long via Phabricator via cfe-commits]

steplong added inline comments.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4841-4850
+  StringRef Level;
+  // Check if argument is prefixed with "-O" or "O"
+  if (Arg.str().rfind("-O", 0) == 0)
+Level = Arg.substr(2);
+  else if (Arg.str().rfind("O", 0) == 0)
+Level = Arg.substr(1);
+  else

aaron.ballman wrote:
> Then, in here, you can parse the `-O` the user passed as a string, 
> and convert it to an `OptimizeAttr::OptLevelKind` enumerator and store that 
> in the semantic attribute.
> 
> This allows you to map things like `-Og` to whatever -O level that actually 
> represents, or do any other kind of mapping that works for you.
> 
> One question we should probably figure out is whether we also want to support 
> clang-cl optimization strings or not. e.g., 
> `__attribute__((optimize("/O0")))` with a slash instead of a dash. Since 
> we're already going to be doing parsing from here anyway, I feel like it 
> might not be a bad idea to also support those. FWIW, here's the list from the 
> user's manual:
> ```
>   /O0 Disable optimization
>   /O1 Optimize for size  (same as /Og /Os /Oy /Ob2 
> /GF /Gy)
>   /O2 Optimize for speed (same as /Og /Oi /Ot /Oy /Ob2 
> /GF /Gy)
>   /Ob0Disable function inlining
>   /Ob1Only inline functions which are (explicitly or 
> implicitly) marked inline
>   /Ob2Inline functions as deemed beneficial by the 
> compiler
>   /Od Disable optimization
>   /Og No effect
>   /Oi-Disable use of builtin functions
>   /Oi Enable use of builtin functions
>   /Os Optimize for size
>   /Ot Optimize for speed
>   /Ox Deprecated (same as /Og /Oi /Ot /Oy /Ob2); use /O2 
> instead
>   /Oy-Disable frame pointer omission (x86 only, default)
>   /Oy Enable frame pointer omission (x86 only)
>   /O   Set multiple /O flags at once; e.g. '/O2y-' for 
> '/O2 /Oy-'
> ```
> (Not all of these would be supported, like enable use of builtin functions, 
> etc.) WDYT?
Hmm I don't think it's necessary to get pragma optimize to work, but it 
shouldn't be hard to add the parsing logic for some of these strings


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126984/new/

https://reviews.llvm.org/D126984

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126984: [clang] Add initial support for gcc's optimize function attribute

[Stephen Long via Phabricator via cfe-commits]

steplong updated this revision to Diff 434639.
steplong added a comment.

- Added docs for attribute
- Changed attribute to use Enum
- Optsize includes -Og, -Oz, and -Ofast
- Change to warning instead of error


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126984/new/

https://reviews.llvm.org/D126984

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/attr-optimize.c
  clang/test/Sema/attr-optimize.c

Index: clang/test/Sema/attr-optimize.c
===
--- /dev/null
+++ clang/test/Sema/attr-optimize.c
@@ -0,0 +1,52 @@
+// RUN: %clang_cc1 -verify -fsyntax-only %s
+
+__attribute__((optimize(a))) // expected-error {{use of undeclared identifier 'a'}}
+void
+f1() {}
+
+int b = 1;
+__attribute__((optimize(b))) // expected-error {{'optimize' attribute requires a string}}
+void
+f2() {}
+
+__attribute__((optimize("O0", "O1"))) // expected-error {{'optimize' attribute takes one argument}}
+void
+f3() {}
+
+__attribute__((optimize("Og"))) // expected-no-error
+void
+f4() {}
+
+__attribute__((optimize("O-1"))) // expected-warning {{invalid optimization level 'O-1' specified; the argument to '-O' should be a non-negative integer, '-Os', '-Oz', '-Og', or '-Ofast'; attribute ignored}}
+void
+f5() {}
+
+__attribute__((optimize("O+1"))) // expected-warning {{invalid optimization level 'O+1' specified; the argument to '-O' should be a non-negative integer, '-Os', '-Oz', '-Og', or '-Ofast'; attribute ignored}}
+void
+f6() {}
+
+__attribute__((optimize("O0"))) // expected-no-error
+void
+f7() {}
+
+__attribute__((optimize("Os"))) // expected-no-error
+void
+f8() {}
+
+__attribute__((optimize("O44"))) // expected-no-error
+void
+f9() {}
+
+__attribute__((optimize("Oz"))) // expected-no-error
+void
+f10() {}
+
+__attribute__((optimize("Ofast"))) // expected-no-error
+void
+f11() {}
+
+__attribute__((optimize("O"))) // expected-no-error
+void
+f12() {}
+
+__attribute__((optimize("O0"))) // expected-error {{expected identifier or '('}}
Index: clang/test/CodeGen/attr-optimize.c
===
--- /dev/null
+++ clang/test/CodeGen/attr-optimize.c
@@ -0,0 +1,28 @@
+// RUN: %clang_cc1 -O2 -S -emit-llvm %s -o - | FileCheck %s --check-prefix=O2
+// RUN: %clang_cc1 -O0 -S -emit-llvm %s -o - | FileCheck %s --check-prefix=O0
+
+__attribute__((optimize("O0"))) void f1(void) {}
+// O2: @f1{{.*}}[[ATTR_OPTNONE:#[0-9]+]]
+// O0: @f1{{.*}}[[ATTR_OPTNONE:#[0-9]+]]
+
+__attribute__((optimize("Os"))) void f2(void) {}
+// O2: @f2{{.*}}[[ATTR_OPTSIZE:#[0-9]+]]
+// O0: @f2{{.*}}[[ATTR_OPTNONE]]
+
+__attribute__((optimize("Og"))) void f3(void) {}
+// O2: @f3{{.*}}[[ATTR_OPTSIZE]]
+// O0: @f3{{.*}}[[ATTR_OPTNONE]]
+
+__attribute__((optimize("Oz"))) void f4(void) {}
+// O2: @f4{{.*}}[[ATTR_OPTSIZE]]
+// O0: @f4{{.*}}[[ATTR_OPTNONE]]
+
+__attribute__((optimize("Ofast"))) void f5(void) {}
+// O2: @f5{{.*}}[[ATTR_OPTSIZE]]
+// O0: @f5{{.*}}[[ATTR_OPTNONE]]
+
+// O2: attributes [[ATTR_OPTNONE]] = { {{.*}}optnone{{.*}} }
+// O2: attributes [[ATTR_OPTSIZE]] = { {{.*}}optsize{{.*}} }
+
+// Check that O0 overrides the attribute
+// O0: attributes [[ATTR_OPTNONE]] = { {{.*}}optnone{{.*}} }
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -46,6 +46,7 @@
 #include "llvm/Support/Error.h"
 #include "llvm/Support/MathExtras.h"
 #include "llvm/Support/raw_ostream.h"
+#include 
 
 using namespace clang;
 using namespace sema;
@@ -4833,6 +4834,48 @@
 D->addAttr(Optnone);
 }
 
+static void handleOptimizeAttr(Sema , Decl *D, const ParsedAttr ) {
+  StringRef Arg;
+  if (!S.checkStringLiteralArgumentAttr(AL, 0, Arg))
+return;
+
+  StringRef Level;
+  // Check if argument is prefixed with "-O" or "O"
+  if (Arg.str().rfind("-O", 0) == 0)
+Level = Arg.substr(2);
+  else if (Arg.str().rfind("O", 0) == 0)
+Level = Arg.substr(1);
+  else
+S.Diag(AL.getLoc(), diag::warn_invalid_optimize_attr_level) << Arg;
+
+  std::unordered_map StrToKind = {
+  {"", OptimizeAttr::O0},  {"s", OptimizeAttr::Os},
+  {"g", OptimizeAttr::Og}, {"fast", OptimizeAttr::Ofast},
+  {"z", OptimizeAttr::Oz}, {"0", OptimizeAttr::O0},
+  {"1", OptimizeAttr::O1}, {"2", OptimizeAttr::O2},
+  {"3", OptimizeAttr::O3}, {"4", OptimizeAttr::O4},
+  };
+
+  DEBUG_WITH_TYPE("foo", llvm::dbgs() << "Level: " << Level << "\n");
+
+  auto It = StrToKind.find(Level.str());
+  if (It != StrToKind.end()) {
+D->addAttr(::new (S.Context) OptimizeAttr(S.Context, AL, "", It->second));
+return;
+  }
+
+  llvm::APInt Num;
+  if (!Level.getAsInteger(10, 

[PATCH] D126984: [clang] Add initial support for gcc's optimize function attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:2265
+  let Spellings = [GCC<"optimize">];
+  let Args = [StringArgument<"Level">];
+  let Subjects = SubjectList<[Function]>;

Something along these lines adds a "fake" argument to the attribute. This means 
the parsed attribute doesn't care about this argument (so users don't supply it 
when writing the attribute themselves), but the semantic attribute 
(`OptimizeAttr`) will have a member to track the fake argument value and will 
require extra information when creating the attribute.

This effectively will add:
```
enum OptLevelKind {
  O0,
  O1,
  O2,
  O3,
  O4
} OptLevel;
```
to the semantic attribute.



Comment at: clang/include/clang/Basic/Attr.td:2267
+  let Subjects = SubjectList<[Function]>;
+  let Documentation = [Undocumented];
+}

You should add some rudimentary documentation for this, and probably point to 
the GCC docs for further information.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:3019-3020
   "%0 attribute takes at least %1 argument%s1">;
+def err_attribute_only_allowed_with_argument : Error <
+  "argument to '%0' should be %1">;
 def err_attribute_invalid_vector_type : Error<"invalid vector element type 
%0">;

I'm not 100% in love with the list of valid options in my suggested wording (I 
had originally listed the valid values manually and that was not much better).

One thing I think is important is that this be a warning rather than an error. 
Users will pass "-f" strings here which are supported by GCC; they should just 
be able to ignore the warning in Clang as being harmless, but if it's an error, 
the user has to change the function signatures in ways that are kind of 
annoying.



Comment at: clang/lib/CodeGen/CGCall.cpp:2169
+HasOptnone = TargetDecl->hasAttr() ||
+ (TargetDecl->hasAttr() &&
+  TargetDecl->getAttr()->getLevel() == "0");

steplong wrote:
> I don't think this is the most ergonomic way. Let me know if you have a 
> better idea of doing this
I'll sprinkle some comments around about how I'd investigate handling this.

Based on those suggestions, here you would be able to ask for 
`OptimizeAttr->getOptLevel()` and it will return the mapped enumeration value, 
which should clean this code up to not require string checking.

Btw, `hasAttr()` followed by `getAttr()` is generally a code smell (same smell 
as `isa` followed by `cast`). You should switch to logic more like:
```
if (const auto *OA = TargetDecl->getAttr()) {

}
```
so that we only have to traverse the list of attributes once instead of twice.



Comment at: clang/lib/CodeGen/CGCall.cpp:2171-2172
+  TargetDecl->getAttr()->getLevel() == "0");
+HasOptsize = TargetDecl->hasAttr() &&
+ TargetDecl->getAttr()->getLevel() == "s";
 if (auto *AllocSize = TargetDecl->getAttr()) {

I think we also need to check for `-Ofast`, `-Oz`, and `-Og` 
(https://github.com/llvm/llvm-project/blob/main/clang/lib/Frontend/CompilerInvocation.cpp#L575)



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4841-4850
+  StringRef Level;
+  // Check if argument is prefixed with "-O" or "O"
+  if (Arg.str().rfind("-O", 0) == 0)
+Level = Arg.substr(2);
+  else if (Arg.str().rfind("O", 0) == 0)
+Level = Arg.substr(1);
+  else

Then, in here, you can parse the `-O` the user passed as a string, 
and convert it to an `OptimizeAttr::OptLevelKind` enumerator and store that in 
the semantic attribute.

This allows you to map things like `-Og` to whatever -O level that actually 
represents, or do any other kind of mapping that works for you.

One question we should probably figure out is whether we also want to support 
clang-cl optimization strings or not. e.g., `__attribute__((optimize("/O0")))` 
with a slash instead of a dash. Since we're already going to be doing parsing 
from here anyway, I feel like it might not be a bad idea to also support those. 
FWIW, here's the list from the user's manual:
```
  /O0 Disable optimization
  /O1 Optimize for size  (same as /Og /Os /Oy /Ob2 /GF 
/Gy)
  /O2 Optimize for speed (same as /Og /Oi /Ot /Oy /Ob2 /GF 
/Gy)
  /Ob0Disable function inlining
  /Ob1Only inline functions which are (explicitly or 
implicitly) marked inline
  /Ob2Inline functions as deemed beneficial by the compiler
  /Od Disable optimization
  /Og No effect
  /Oi-Disable use of builtin functions
  /Oi Enable use of builtin functions
  /Os Optimize for size
  /Ot Optimize for speed
  /Ox Deprecated (same as 

[PATCH] D126984: [clang] Add initial support for gcc's optimize function attribute

[Stephen Long via Phabricator via cfe-commits]

steplong added inline comments.



Comment at: clang/lib/CodeGen/CGCall.cpp:2169
+HasOptnone = TargetDecl->hasAttr() ||
+ (TargetDecl->hasAttr() &&
+  TargetDecl->getAttr()->getLevel() == "0");

I don't think this is the most ergonomic way. Let me know if you have a better 
idea of doing this


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126984/new/

https://reviews.llvm.org/D126984

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126984: [clang] Add initial support for gcc's optimize function attribute

[Stephen Long via Phabricator via cfe-commits]

steplong created this revision.
Herald added a reviewer: aaron.ballman.
Herald added a project: All.
steplong requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

At the moment, it only supports a string argument that refers to
the optimization level. "-f" arguments aren't supported. The argument
must begin with "O" or "-O". Also, only non-negative optimization levels
and "-Os" are accepted.

Only "-Os" and "-O0" currently affect codegen.

i.e. __attribute__((optimize("O0")))


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D126984

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/attr-optimize.c
  clang/test/Sema/attr-optimize.c

Index: clang/test/Sema/attr-optimize.c
===
--- /dev/null
+++ clang/test/Sema/attr-optimize.c
@@ -0,0 +1,31 @@
+// RUN: %clang_cc1 -verify -fsyntax-only %s
+
+__attribute__((optimize(a))) // expected-error {{use of undeclared identifier 'a'}}
+void f1() {}
+
+int b = 1;
+__attribute__((optimize(b))) // expected-error {{'optimize' attribute requires a string}}
+void f2() {}
+
+__attribute__((optimize("O0", "O1"))) // expected-error {{'optimize' attribute takes one argument}}
+void f3() {}
+
+__attribute__((optimize("Og"))) // expected-error {{argument to '-O' should be non-negative integer or 's'}}
+void f4() {}
+
+__attribute__((optimize("O-1"))) // expected-error {{argument to '-O' should be non-negative integer or 's'}}
+void f5() {}
+
+__attribute__((optimize("O+1"))) // expected-error {{argument to '-O' should be non-negative integer or 's'}}
+void f6() {}
+
+__attribute__((optimize("O0"))) // expected-no-error
+void f7() {}
+
+__attribute__((optimize("Os"))) // expected-no-error
+void f8() {}
+
+__attribute__((optimize("O44"))) // expected-no-error
+void f9() {}
+
+__attribute__((optimize("O0"))) // expected-error {{expected identifier or '('}}
Index: clang/test/CodeGen/attr-optimize.c
===
--- /dev/null
+++ clang/test/CodeGen/attr-optimize.c
@@ -0,0 +1,16 @@
+// RUN: %clang_cc1 -O2 -S -emit-llvm %s -o - | FileCheck %s --check-prefix=O2
+// RUN: %clang_cc1 -O0 -S -emit-llvm %s -o - | FileCheck %s --check-prefix=O0
+
+__attribute__((optimize("O0"))) void f1(void) {}
+// O2: @f1{{.*}}[[ATTR1:#[0-9]+]]
+// O0: @f1{{.*}}[[ATTR1:#[0-9]+]]
+
+__attribute__((optimize("Os"))) void f2(void) {}
+// O2: @f2{{.*}}[[ATTR2:#[0-9]+]]
+// O0: @f2{{.*}}[[ATTR1]]
+
+// O2: attributes [[ATTR1]] = { {{.*}}optnone{{.*}} }
+// O2: attributes [[ATTR2]] = { {{.*}}optsize{{.*}} }
+
+// Check that O0 overrides the attribute
+// O0: attributes [[ATTR1]] = { {{.*}}optnone{{.*}} }
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -4833,6 +4833,43 @@
 D->addAttr(Optnone);
 }
 
+static void handleOptimizeAttr(Sema , Decl *D, const ParsedAttr ) {
+  StringRef Arg;
+  if (!S.checkStringLiteralArgumentAttr(AL, 0, Arg))
+return;
+
+  StringRef Level;
+  // Check if argument is prefixed with "-O" or "O"
+  if (Arg.str().rfind("-O", 0) == 0)
+Level = Arg.substr(2);
+  else if (Arg.str().rfind("O", 0) == 0)
+Level = Arg.substr(1);
+  else
+S.Diag(AL.getLoc(), diag::err_attribute_only_allowed_with_argument)
+<< "-O"
+<< "non-negative integer or 's'";
+
+  llvm::APInt Num;
+  bool IsNum = !Level.getAsInteger(10, Num);
+
+  if (Level == "" || (IsNum && Num == 0)) {
+D->addAttr(::new (S.Context) OptimizeAttr(S.Context, AL, "0"));
+return;
+  } else if (Level == "s") {
+D->addAttr(::new (S.Context) OptimizeAttr(S.Context, AL, "s"));
+return;
+  } else if (IsNum) {
+// Limit level to -O3 if higher
+std::string Level = std::to_string(Num.getLimitedValue(3));
+D->addAttr(::new (S.Context) OptimizeAttr(S.Context, AL, Level));
+return;
+  }
+
+  S.Diag(AL.getLoc(), diag::err_attribute_only_allowed_with_argument)
+  << "-O"
+  << "non-negative integer or 's'";
+}
+
 static void handleConstantAttr(Sema , Decl *D, const ParsedAttr ) {
   const auto *VD = cast(D);
   if (VD->hasLocalStorage()) {
@@ -8525,6 +8562,9 @@
   case ParsedAttr::AT_OptimizeNone:
 handleOptimizeNoneAttr(S, D, AL);
 break;
+  case ParsedAttr::AT_Optimize:
+handleOptimizeAttr(S, D, AL);
+break;
   case ParsedAttr::AT_EnumExtensibility:
 handleEnumExtensibilityAttr(S, D, AL);
 break;
Index: clang/lib/CodeGen/CodeGenModule.h
===
--- clang/lib/CodeGen/CodeGenModule.h
+++ clang/lib/CodeGen/CodeGenModule.h
@@ -1654,7 +1654,7 @@
   /// addDefaultFunctionDefinitionAttributes.