[PATCH] D125723: [MSVC] Add support for MSVC pragma optimize

[Stephen Long via Phabricator via cfe-commits]

steplong updated this revision to Diff 436954.
steplong edited the summary of this revision.
steplong added a comment.

- Change logic to only handle empty optimization list
- Fixup doc and tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125723

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/Sema.h
  clang/lib/Parse/ParsePragma.cpp
  clang/lib/Sema/SemaAttr.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/test/CodeGen/pragma-msvc-optimize.c
  clang/test/Preprocessor/pragma_microsoft.c

Index: clang/test/Preprocessor/pragma_microsoft.c
===
--- clang/test/Preprocessor/pragma_microsoft.c
+++ clang/test/Preprocessor/pragma_microsoft.c
@@ -228,7 +228,13 @@
 #pragma optimize("g"  // expected-warning{{expected ',' in '#pragma optimize'}}
 #pragma optimize("g", // expected-warning{{missing argument to '#pragma optimize'; expected 'on' or 'off'}}
 #pragma optimize("g",xyz  // expected-warning{{unexpected argument 'xyz' to '#pragma optimize'; expected 'on' or 'off'}}
-#pragma optimize("g",on)  // expected-warning{{#pragma optimize' is not supported}}
+#pragma optimize("g",on)  // expected-warning{{unexpected argument 'g' to '#pragma optimize'; expected ""}}
+#pragma optimize("",on)  // no-warning
+#pragma optimize("", on) asdf // expected-warning{{extra tokens at end of '#pragma optimize'}}
+
+void pragma_optimize_foo() {
+#pragma optimize("", on) // expected-error {{'#pragma optimize' can only appear at file scope}}
+}
 
 #pragma execution_character_set // expected-warning {{expected '('}}
 #pragma execution_character_set(// expected-warning {{expected 'push' or 'pop'}}
Index: clang/test/CodeGen/pragma-msvc-optimize.c
===
--- /dev/null
+++ clang/test/CodeGen/pragma-msvc-optimize.c
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 -O2 -emit-llvm -fms-extensions -o - %s | FileCheck %s
+
+#pragma optimize("", off)
+
+// CHECK: define{{.*}} void @f0(){{.*}} #[[OPTNONE:[0-9]+]]
+void f0() {}
+
+// CHECK: define{{.*}} void @f1(){{.*}} #[[OPTNONE]]
+void f1() {}
+
+#pragma optimize("", on)
+
+// CHECK: define{{.*}} void @f2(){{.*}} #[[NO_OPTNONE:[0-9]+]]
+void f2() {}
+
+// CHECK: define{{.*}} void @f3(){{.*}} #[[NO_OPTNONE]]
+void f3() {}
+
+// CHECK: attributes #[[OPTNONE]] = {{{.*}}optnone{{.*}}}
+// CHECK-NOT: attributes #[[NO_OPTNONE]] = {{{.*}}optnone{{.*}}}
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -10192,6 +10192,7 @@
 AddRangeBasedOptnone(NewFD);
 AddImplicitMSFunctionNoBuiltinAttr(NewFD);
 AddSectionMSAllocText(NewFD);
+ModifyFnAttributesMSPragmaOptimize(NewFD);
   }
 
   // If this is the first declaration of an extern C variable, update
Index: clang/lib/Sema/SemaAttr.cpp
===
--- clang/lib/Sema/SemaAttr.cpp
+++ clang/lib/Sema/SemaAttr.cpp
@@ -1096,6 +1096,15 @@
 OptimizeOffPragmaLocation = PragmaLoc;
 }
 
+void Sema::ActOnPragmaMSOptimize(SourceLocation Loc, bool IsOn) {
+  if (!CurContext->getRedeclContext()->isFileContext()) {
+Diag(Loc, diag::err_pragma_expected_file_scope) << "optimize";
+return;
+  }
+
+  MSPragmaOptimizeIsOn = IsOn;
+}
+
 void Sema::ActOnPragmaMSFunction(
 SourceLocation Loc, const llvm::SmallVectorImpl &NoBuiltins) {
   if (!CurContext->getRedeclContext()->isFileContext()) {
@@ -1129,6 +1138,17 @@
   }
 }
 
+void Sema::ModifyFnAttributesMSPragmaOptimize(FunctionDecl *FD) {
+  assert(MSPragmaOptimizeValidParams.empty());
+
+  // Don't modify the function attributes if it's "on". "on" resets the
+  // optimizations to the ones listed on the command line
+  if (!MSPragmaOptimizeIsOn) {
+AddOptnoneAttributeIfNoConflicts(FD, FD->getBeginLoc());
+return;
+  }
+}
+
 void Sema::AddOptnoneAttributeIfNoConflicts(FunctionDecl *FD,
 SourceLocation Loc) {
   // Don't add a conflicting attribute. No diagnostic is needed.
Index: clang/lib/Parse/ParsePragma.cpp
===
--- clang/lib/Parse/ParsePragma.cpp
+++ clang/lib/Parse/ParsePragma.cpp
@@ -255,12 +255,6 @@
 Token &FirstToken) override;
 };
 
-struct PragmaMSOptimizeHandler : public PragmaHandler {
-  PragmaMSOptimizeHandler() : PragmaHandler("optimize") {}
-  void HandlePragma(Preprocessor &PP, PragmaIntroducer Introducer,
-Token &FirstToken) override;
-};
-
 // "\#pragma fenv_access (on)".
 struct PragmaMSFenvAccessHandler : public PragmaHandler {
   PragmaMSFenvAccessHandler() : PragmaHandler("fenv_access") {}
@@ -449,12 +443,1

[PATCH] D125723: [MSVC] Add support for MSVC pragma optimize

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

xbolva00 added a comment.

>> I would like to hear quite strong motivating words.

From https://reviews.llvm.org/D127565, Aaron mentioned "we have > 400 semantic 
attributes already and the support matrix for their combinations is pretty bad. 
". Yes, this is a real good motivation why to rework these things.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125723

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

[PATCH] D125723: [MSVC] Add support for MSVC pragma optimize

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

xbolva00 added a comment.

In D125723#3547789 , @rnk wrote:

> I think it's fine to implement this, but I wanted to share some of the 
> motivation for the current state of things. In our experience, the majority 
> of uses of pragma optimize were to work around MSVC compiler bugs. So, 
> instead of honoring them, we felt it was best to ignore them with a warning 
> (`-Wignored-pragma-optimize`). Of course, that warning is typically disabled 
> in build systems of large projects, so our current behavior can still 
> surprise users. Implementing the feature is probably the most predictable and 
> least surprising thing Clang can do.
>
> Something we can't easily support for either GCC or MSVC attribute is 
> enabling optimizations for some functions in an -O0 build. Maybe it's now 
> possible to set up a full pass pipeline after semantic analysis is complete, 
> but it's not straightforward. You should consider what to do with that corner 
> case.

What is a real motivation to have it in Clang as well? As you said, for MSVC, 
to work around bugs. You may say optimise(-Oz), but we have attribute minsize 
for that. Just to match MSVC?

As shown https://reviews.llvm.org/D126984, things work a bit different in Clang 
vs MSVC/GCC so to justify this pragma, optimise attribute (and related (huge, 
imho) rework to move other attributes under OptimiseAttr, maybe rework of LLVM 
attributes), I would like to hear quite strong motivating words.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125723

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

[PATCH] D125723: [MSVC] Add support for MSVC pragma optimize

[Stephen Long via Phabricator via cfe-commits]

steplong added inline comments.



Comment at: clang/include/clang/Sema/Sema.h:768
+  /// (i.e. `ModifyFnAttributeMSPragmaOptimze()` does nothing)
+  bool MSPragmaOptimizeIsOn = true;
+

This might be confusing. `ModifyFnAttributeMSPragmaOptimize()` adds optnone to 
functions if `MSPragmaOptimizeIsOn` is false and `MSPragmaOptimizeValidParams` 
is empty.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125723

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

[PATCH] D125723: [MSVC] Add support for MSVC pragma optimize

[Stephen Long via Phabricator via cfe-commits]

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

- Parse the string in the pragma
- Some of the tests should fail. I'll fix them up once D126984 
 looks good


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125723

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/Sema.h
  clang/lib/Parse/ParsePragma.cpp
  clang/lib/Sema/SemaAttr.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/test/CodeGen/pragma-msvc-optimize.c
  clang/test/Preprocessor/pragma_microsoft.c

Index: clang/test/Preprocessor/pragma_microsoft.c
===
--- clang/test/Preprocessor/pragma_microsoft.c
+++ clang/test/Preprocessor/pragma_microsoft.c
@@ -228,7 +228,12 @@
 #pragma optimize("g"  // expected-warning{{expected ',' in '#pragma optimize'}}
 #pragma optimize("g", // expected-warning{{missing argument to '#pragma optimize'; expected 'on' or 'off'}}
 #pragma optimize("g",xyz  // expected-warning{{unexpected argument 'xyz' to '#pragma optimize'; expected 'on' or 'off'}}
-#pragma optimize("g",on)  // expected-warning{{#pragma optimize' is not supported}}
+#pragma optimize("g",on)  // no-warning
+#pragma optimize("g", on) asdf // expected-warning{{extra tokens at end of '#pragma optimize'}}
+
+void pragma_optimize_foo() {
+#pragma optimize("g", on) // expected-error {{'#pragma optimize' can only appear at file scope}}
+}
 
 #pragma execution_character_set // expected-warning {{expected '('}}
 #pragma execution_character_set(// expected-warning {{expected 'push' or 'pop'}}
Index: clang/test/CodeGen/pragma-msvc-optimize.c
===
--- /dev/null
+++ clang/test/CodeGen/pragma-msvc-optimize.c
@@ -0,0 +1,48 @@
+// RUN: %clang_cc1 -O2 -emit-llvm -fms-extensions -o - %s | FileCheck %s -check-prefix CHECK-O2
+// RUN: %clang_cc1 -O0 -emit-llvm -fms-extensions -o - %s | FileCheck %s -check-prefix CHECK-O0
+// RUN: %clang_cc1 -Os -emit-llvm -fms-extensions -o - %s | FileCheck %s -check-prefix CHECK-Os
+
+#pragma optimize("s", on)
+
+// CHECK-O2: define{{.*}} void @f0(){{.*}} #[[OPTSIZE:[0-9]+]]
+void f0() {}
+
+#pragma optimize("y", on)
+
+// CHECK-O2: define{{.*}} void @f1(){{.*}} #[[OPTSIZE_FRAMEPTR_NONLEAF:[0-9]+]]
+void f1() {}
+
+#pragma optimize("t", on)
+
+// CHECK-O2: define{{.*}} void @f2(){{.*}} #[[OPTSIZE_FRAMEPTR_NONLEAF]]
+// CHECK-O0: define{{.*}} void @f2(){{.*}} #[[NO_OPTNONE:[0-9]+]]
+void f2() {}
+
+#pragma optimize("s", off)
+
+// CHECK-O2: define{{.*}} void @f3(){{.*}} #[[FRAMEPTR_NONLEAF:[0-9]+]]
+// CHECK-Os: define{{.*}} void @f3(){{.*}} #[[FRAMEPTR_NONLEAF:[0-9]+]]
+void f3() {}
+
+#pragma optimize("y", off)
+
+// CHECK-O2: define{{.*}} void @f4(){{.*}} #[[FRAMEPTR_NONE:[0-9]+]]
+void f4() {}
+
+#pragma optimize("t", off)
+
+// CHECK-O2: define{{.*}} void @f5(){{.*}} #[[OPTNONE_FRAMEPTR_NONE:[0-9]+]]
+void f5() {}
+
+// CHECK-O2: attributes #[[OPTSIZE]] = {{{.*}}optsize{{.*}}}
+// CHECK-O2-NOT: attributes #[[OPTSIZE]] = {{{.*}}optsize{{.*}}"frame-pointer"="non-leaf"{{.*}}}
+// CHECK-O2: attributes #[[OPTSIZE_FRAMEPTR_NONLEAF]] = {{{.*}}optsize{{.*}}"frame-pointer"="non-leaf"{{.*}}}
+// CHECK-O2: attributes #[[FRAMEPTR_NONLEAF]] = {{{.*}}"frame-pointer"="non-leaf"{{.*}}}
+// CHECK-O2-NOT: attributes #[[FRAMEPTR_NONLEAF]] = {{{.*}}optsize{{.*}}"frame-pointer"="non-leaf"{{.*}}}
+// CHECK-O2: attributes #[[FRAMEPTR_NONE]] = {{{.*}}"frame-pointer"="none"{{.*}}}
+// CHECK-O2: attributes #[[OPTNONE_FRAMEPTR_NONE]] = {{{.*}}optnone{{.*}}"frame-pointer"="none"{{.*}}}
+
+// CHECK-O0-NOT: attributes #[[NO_OPTNONE]] = {{{.*}}optnone{{.*}}}
+
+// CHECK-Os: attributes #[[FRAMEPTR_NONLEAF]] = {{{.*}}"frame-pointer"="non-leaf"{{.*}}}
+// CHECK-Os-NOT: attributes #[[FRAMEPTR_NONLEAF]] = {{{.*}}optsize{{.*}}"frame-pointer"="non-leaf"{{.*}}}
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -10192,6 +10192,7 @@
 AddRangeBasedOptnone(NewFD);
 AddImplicitMSFunctionNoBuiltinAttr(NewFD);
 AddSectionMSAllocText(NewFD);
+ModifyFnAttributesMSPragmaOptimize(NewFD);
   }
 
   // If this is the first declaration of an extern C variable, update
Index: clang/lib/Sema/SemaAttr.cpp
===
--- clang/lib/Sema/SemaAttr.cpp
+++ clang/lib/Sema/SemaAttr.cpp
@@ -1096,6 +1096,17 @@
 OptimizeOffPragmaLocation = PragmaLoc;
 }
 
+void Sema::ActOnPragmaMSOptimize(SourceLocation Loc, bool IsOn,
+ std::string ValidOptimizationList) {
+  if (!CurContext->getRedeclContext()->isFileContext()) {
+Diag(Loc, diag::err_pragma_expected_fil

[PATCH] D125723: [MSVC] Add support for MSVC pragma optimize

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D125723#3553705 , @rnk wrote:

> In D125723#3553664 , @aaron.ballman 
> wrote:
>
>> In D125723#3550956 , @steplong 
>> wrote:
>>
>>> Appreciate the help! It's not clear to me how to go from the strings "Os", 
>>> "foo1", "foo2", "foo3" to adding "-Os -ffoo1 -ffoo2 -ffoo3" to the 
>>> compilation line for that function
>>
>> I may be misunderstanding the issue here, but what I had envisioned was that 
>> the `OptimizeAttr` would take either a string or integer argument (I don't 
>> know how easy that will be to model in Attr.td, FWIW; it may require adding 
>> a new kind of `Argument` for attributes) and then add a "fake" `Argument` or 
>> use the `AdditionalMembers` field to add a member that stores the converted 
>> "value" of that string or integer in whatever form makes the most sense for 
>> the semantics (I was envisioning an enumeration for the various kinds of 
>> optimization options, but maybe that doesn't work for the -f arguments?). 
>> Then, when doing CodeGen, you can look at the function to see if it has an 
>> `OptimizeAttr`, and if it does, use the fake/additional member to determine 
>> what information to lower to IR (or not lower, as the case may be). Does 
>> that help get you unstuck somewhat?
>>
>> Btw, one possibility would be to not support any -f flags initially and only 
>> support optimization levels, if that's easier. We can add support for 
>> individual flags at a later step but still get utility out of the attribute 
>> this way.
>
> I would really like to limit the scope here to foreclose any future 
> possibility of sending attribute strings through the command line parser. 
> That would expose a big, general API surface, with lots of possibilities for 
> confusing interactions with other features.

Agreed.

> I think it will be cleaner to keep the attributes as close as possible to 
> simple booleans, which then correspond relatively directly to the LLVM IR 
> `optnone`, `optsize`, etc attributes.

Also agreed. One thing I was double-checking and am 99% sure of is that we 
basically can't support those -f options in Clang anyway and so we likely just 
want to eat them (silently so we don't cause issues when cross compiling with 
GCC?). Clang does not have a mapping between -O flags and -f flags in the same 
way that GCC does. Also, it's not clear to me that there's always a way to 
specify the -f flag effects for only a single function. Given that GCC 
documents the `optimize` attribute as "The optimize attribute should be used 
for debugging purposes only. It is not suitable in production code.", I think 
it's reasonable just to punt on -f flags entirely.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125723

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

[PATCH] D125723: [MSVC] Add support for MSVC pragma optimize

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

In D125723#3553664 , @aaron.ballman 
wrote:

> In D125723#3550956 , @steplong 
> wrote:
>
>> Appreciate the help! It's not clear to me how to go from the strings "Os", 
>> "foo1", "foo2", "foo3" to adding "-Os -ffoo1 -ffoo2 -ffoo3" to the 
>> compilation line for that function
>
> I may be misunderstanding the issue here, but what I had envisioned was that 
> the `OptimizeAttr` would take either a string or integer argument (I don't 
> know how easy that will be to model in Attr.td, FWIW; it may require adding a 
> new kind of `Argument` for attributes) and then add a "fake" `Argument` or 
> use the `AdditionalMembers` field to add a member that stores the converted 
> "value" of that string or integer in whatever form makes the most sense for 
> the semantics (I was envisioning an enumeration for the various kinds of 
> optimization options, but maybe that doesn't work for the -f arguments?). 
> Then, when doing CodeGen, you can look at the function to see if it has an 
> `OptimizeAttr`, and if it does, use the fake/additional member to determine 
> what information to lower to IR (or not lower, as the case may be). Does that 
> help get you unstuck somewhat?
>
> Btw, one possibility would be to not support any -f flags initially and only 
> support optimization levels, if that's easier. We can add support for 
> individual flags at a later step but still get utility out of the attribute 
> this way.

I would really like to limit the scope here to foreclose any future possibility 
of sending attribute strings through the command line parser. That would expose 
a big, general API surface, with lots of possibilities for confusing 
interactions with other features.

I think it will be cleaner to keep the attributes as close as possible to 
simple booleans, which then correspond relatively directly to the LLVM IR 
`optnone`, `optsize`, etc attributes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125723

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

[PATCH] D125723: [MSVC] Add support for MSVC pragma optimize

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D125723#3550956 , @steplong wrote:

> Appreciate the help! It's not clear to me how to go from the strings "Os", 
> "foo1", "foo2", "foo3" to adding "-Os -ffoo1 -ffoo2 -ffoo3" to the 
> compilation line for that function

I may be misunderstanding the issue here, but what I had envisioned was that 
the `OptimizeAttr` would take either a string or integer argument (I don't know 
how easy that will be to model in Attr.td, FWIW; it may require adding a new 
kind of `Argument` for attributes) and then add a "fake" `Argument` or use the 
`AdditionalMembers` field to add a member that stores the converted "value" of 
that string or integer in whatever form makes the most sense for the semantics 
(I was envisioning an enumeration for the various kinds of optimization 
options, but maybe that doesn't work for the -f arguments?). Then, when doing 
CodeGen, you can look at the function to see if it has an `OptimizeAttr`, and 
if it does, use the fake/additional member to determine what information to 
lower to IR (or not lower, as the case may be). Does that help get you unstuck 
somewhat?

Btw, one possibility would be to not support any -f flags initially and only 
support optimization levels, if that's easier. We can add support for 
individual flags at a later step but still get utility out of the attribute 
this way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125723

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

[PATCH] D125723: [MSVC] Add support for MSVC pragma optimize

[Stephen Long via Phabricator via cfe-commits]

steplong added a comment.

Appreciate the help! It's not clear to me how to go from the strings "Os", 
"foo1", "foo2", "foo3" to adding "-Os -ffoo1 -ffoo2 -ffoo3" to the compilation 
line for that function


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125723

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

[PATCH] D125723: [MSVC] Add support for MSVC pragma optimize

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/Sema/SemaAttr.cpp:1207-1213
+  FD->addAttr(FramePointerAttr::CreateImplicit(Context, Kind));
+}
+
+void Sema::AddOptsize(FunctionDecl *FD, SourceLocation Loc) {
+  FD->dropAttr();
+  OptimizeSizeAttr::Kind Kind = OptimizeSizeAttr::On;
+  FD->addAttr(OptimizeSizeAttr::CreateImplicit(Context, Kind));

steplong wrote:
> aaron.ballman wrote:
> > Rather than creating two new, implicit attributes for this, why not add 
> > support for `__attribute__((optimize(...)))` from GCC and reuse that one?
> > 
> > https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#Common-Function-Attributes
> > 
> > It seems like that serves the same function as these implicit attributes, 
> > but then users would get two features for the price of one.
> Hmm that makes sense. So, `pragma optimize(on, "s")` would be equivalent to 
> `__attribute__((optimize("-Os"))`. Could you give me a brief description of 
> what I would have to do to implement the attribute? I'm new to this
> Hmm that makes sense. So, pragma optimize(on, "s") would be equivalent to 
> __attribute__((optimize("-Os")).

Yup, effectively! And in terms of the AST, your pragma can implicitly create an 
`OptimizeAttr` with the appropriate arguments; then everything works off the 
same machinery.

> Could you give me a brief description of what I would have to do to implement 
> the attribute? I'm new to this

Absolutely! 
https://clang.llvm.org/docs/InternalsManual.html#how-to-add-an-attribute has 
most of the information you need. I'm happy to give you more information if 
you'd like it, though.



Comment at: clang/lib/Sema/SemaAttr.cpp:1224
+  FD->dropAttr();
+  FD->addAttr(NeverOptimizeNoneAttr::CreateImplicit(Context));
+}

steplong wrote:
> aaron.ballman wrote:
> > Can you explain why this is needed?
> I added this attr, so we can remove the attr optnone even if -O0 was passed 
> on the cmdline
I think if you add an `OptimizeAttr`, that could be used to carry this 
information instead of using a dedicated attribute for just the "never optnone" 
part.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125723

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

[PATCH] D125723: [MSVC] Add support for MSVC pragma optimize

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

I think it's fine to implement this, but I wanted to share some of the 
motivation for the current state of things. In our experience, the majority of 
uses of pragma optimize were to work around MSVC compiler bugs. So, instead of 
honoring them, we felt it was best to ignore them with a warning 
(`-Wignored-pragma-optimize`). Of course, that warning is typically disabled in 
build systems of large projects, so our current behavior can still surprise 
users. Implementing the feature is probably the most predictable and least 
surprising thing Clang can do.

Something we can't easily support for either GCC or MSVC attribute is enabling 
optimizations for some functions in an -O0 build. Maybe it's now possible to 
set up a full pass pipeline after semantic analysis is complete, but it's not 
straightforward. You should consider what to do with that corner case.




Comment at: clang/lib/Parse/ParsePragma.cpp:3706
-  }
-  PP.Diag(StartLoc, diag::warn_pragma_optimize);
 }

This diagnostic is probably dead now, please remove it from the .td file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125723

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

[PATCH] D125723: [MSVC] Add support for MSVC pragma optimize

[Stephen Long via Phabricator via cfe-commits]

steplong added inline comments.



Comment at: clang/lib/Sema/SemaAttr.cpp:1207-1213
+  FD->addAttr(FramePointerAttr::CreateImplicit(Context, Kind));
+}
+
+void Sema::AddOptsize(FunctionDecl *FD, SourceLocation Loc) {
+  FD->dropAttr();
+  OptimizeSizeAttr::Kind Kind = OptimizeSizeAttr::On;
+  FD->addAttr(OptimizeSizeAttr::CreateImplicit(Context, Kind));

aaron.ballman wrote:
> Rather than creating two new, implicit attributes for this, why not add 
> support for `__attribute__((optimize(...)))` from GCC and reuse that one?
> 
> https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#Common-Function-Attributes
> 
> It seems like that serves the same function as these implicit attributes, but 
> then users would get two features for the price of one.
Hmm that makes sense. So, `pragma optimize(on, "s")` would be equivalent to 
`__attribute__((optimize("-Os"))`. Could you give me a brief description of 
what I would have to do to implement the attribute? I'm new to this



Comment at: clang/lib/Sema/SemaAttr.cpp:1224
+  FD->dropAttr();
+  FD->addAttr(NeverOptimizeNoneAttr::CreateImplicit(Context));
+}

aaron.ballman wrote:
> Can you explain why this is needed?
I added this attr, so we can remove the attr optnone even if -O0 was passed on 
the cmdline


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125723

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

[PATCH] D125723: [MSVC] Add support for MSVC pragma optimize

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/Sema/SemaAttr.cpp:1207-1213
+  FD->addAttr(FramePointerAttr::CreateImplicit(Context, Kind));
+}
+
+void Sema::AddOptsize(FunctionDecl *FD, SourceLocation Loc) {
+  FD->dropAttr();
+  OptimizeSizeAttr::Kind Kind = OptimizeSizeAttr::On;
+  FD->addAttr(OptimizeSizeAttr::CreateImplicit(Context, Kind));

Rather than creating two new, implicit attributes for this, why not add support 
for `__attribute__((optimize(...)))` from GCC and reuse that one?

https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#Common-Function-Attributes

It seems like that serves the same function as these implicit attributes, but 
then users would get two features for the price of one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125723

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

[PATCH] D125723: [MSVC] Add support for MSVC pragma optimize

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/Parse/ParsePragma.cpp:3662
+  }
+  ExprResult StringResult = Parser::ParseStringLiteralExpression();
+  if (StringResult.isInvalid())





Comment at: clang/lib/Parse/ParsePragma.cpp:3671
   }
   // We could syntax check the string but it's probably not worth the effort.
 

I think we should be syntax checking the string. Microsoft doesn't, but... 
what's the benefit to silently doing nothing?



Comment at: clang/lib/Sema/SemaAttr.cpp:1099
 
+void Sema::ActOnPragmaMSOptimize(SourceLocation Loc, bool On,
+ StringRef OptimizationList) {

I got tripped up below by seeing `[On]` and thinking it only ever looped over 
the "enabled" items in the list.



Comment at: clang/lib/Sema/SemaAttr.cpp:1108
+MSOptimizeOperations.clear();
+if (!On) {
+  MSOptimizeOperations.push_back(&Sema::AddOptnoneAttributeIfNoConflicts);

It looks like we're not handling this bit of the MS documentation: 
```
Using the optimize pragma with the empty string ("") is a special form of the 
directive:

...

When you use the on parameter, it resets the optimizations to the ones that you 
specified using the /O compiler option.
```



Comment at: clang/lib/Sema/SemaAttr.cpp:1224
+  FD->dropAttr();
+  FD->addAttr(NeverOptimizeNoneAttr::CreateImplicit(Context));
+}

Can you explain why this is needed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125723

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

[PATCH] D125723: [MSVC] Add support for MSVC pragma optimize

[Stephen Long via Phabricator via cfe-commits]

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

MSVC's pragma optimize turns optimizations on or off based on the list
passed. Depends on D125722 .

>From MSVC's docs:
+---+-+

| Parameter | Type of optimization |
|

+---+-+

| g | Enable global optimizations. Deprecated |
|

+---+-+

| s or t | Specify short or fast sequences of machine code |
|

+---+-+

| y | Generate frame pointers on the program stack |
|

+---+-+

https://docs.microsoft.com/en-us/cpp/preprocessor/optimize?view=msvc-170


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D125723

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/Sema.h
  clang/lib/Parse/ParsePragma.cpp
  clang/lib/Sema/SemaAttr.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/test/CodeGen/pragma-msvc-optimize.c
  clang/test/Preprocessor/pragma_microsoft.c

Index: clang/test/Preprocessor/pragma_microsoft.c
===
--- clang/test/Preprocessor/pragma_microsoft.c
+++ clang/test/Preprocessor/pragma_microsoft.c
@@ -228,7 +228,12 @@
 #pragma optimize("g"  // expected-warning{{expected ',' in '#pragma optimize'}}
 #pragma optimize("g", // expected-warning{{missing argument to '#pragma optimize'; expected 'on' or 'off'}}
 #pragma optimize("g",xyz  // expected-warning{{unexpected argument 'xyz' to '#pragma optimize'; expected 'on' or 'off'}}
-#pragma optimize("g",on)  // expected-warning{{#pragma optimize' is not supported}}
+#pragma optimize("g",on)  // no-warning
+#pragma optimize("g", on) asdf // expected-warning{{extra tokens at end of '#pragma optimize'}}
+
+void pragma_optimize_foo() {
+#pragma optimize("g", on) // expected-error {{'#pragma optimize' can only appear at file scope}}
+}
 
 #pragma execution_character_set // expected-warning {{expected '('}}
 #pragma execution_character_set(// expected-warning {{expected 'push' or 'pop'}}
Index: clang/test/CodeGen/pragma-msvc-optimize.c
===
--- /dev/null
+++ clang/test/CodeGen/pragma-msvc-optimize.c
@@ -0,0 +1,48 @@
+// RUN: %clang_cc1 -O2 -emit-llvm -fms-extensions -o - %s | FileCheck %s -check-prefix CHECK-O2
+// RUN: %clang_cc1 -O0 -emit-llvm -fms-extensions -o - %s | FileCheck %s -check-prefix CHECK-O0
+// RUN: %clang_cc1 -Os -emit-llvm -fms-extensions -o - %s | FileCheck %s -check-prefix CHECK-Os
+
+#pragma optimize("s", on)
+
+// CHECK-O2: define{{.*}} void @f0(){{.*}} #[[OPTSIZE:[0-9]+]]
+void f0() {}
+
+#pragma optimize("y", on)
+
+// CHECK-O2: define{{.*}} void @f1(){{.*}} #[[OPTSIZE_FRAMEPTR_NONLEAF:[0-9]+]]
+void f1() {}
+
+#pragma optimize("t", on)
+
+// CHECK-O2: define{{.*}} void @f2(){{.*}} #[[OPTSIZE_FRAMEPTR_NONLEAF]]
+// CHECK-O0: define{{.*}} void @f2(){{.*}} #[[NO_OPTNONE:[0-9]+]]
+void f2() {}
+
+#pragma optimize("s", off)
+
+// CHECK-O2: define{{.*}} void @f3(){{.*}} #[[FRAMEPTR_NONLEAF:[0-9]+]]
+// CHECK-Os: define{{.*}} void @f3(){{.*}} #[[FRAMEPTR_NONLEAF:[0-9]+]]
+void f3() {}
+
+#pragma optimize("y", off)
+
+// CHECK-O2: define{{.*}} void @f4(){{.*}} #[[FRAMEPTR_NONE:[0-9]+]]
+void f4() {}
+
+#pragma optimize("t", off)
+
+// CHECK-O2: define{{.*}} void @f5(){{.*}} #[[OPTNONE_FRAMEPTR_NONE:[0-9]+]]
+void f5() {}
+
+// CHECK-O2: attributes #[[OPTSIZE]] = {{{.*}}optsize{{.*}}}
+// CHECK-O2-NOT: attributes #[[OPTSIZE]] = {{{.*}}optsize{{.*}}"frame-pointer"="non-leaf"{{.*}}}
+// CHECK-O2: attributes #[[OPTSIZE_FRAMEPTR_NONLEAF]] = {{{.*}}optsize{{.*}}"frame-pointer"="non-leaf"{{.*}}}
+// CHECK-O2: attributes #[[FRAMEPTR_NONLEAF]] = {{{.*}}"frame-pointer"="non-leaf"{{.*}}}
+// CHECK-O2-NOT: attributes #[[FRAMEPTR_NONLEAF]] = {{{.*}}optsize{{.*}}"frame-pointer"="non-leaf"{{.*}}}
+// CHECK-O2: attributes #[[FRAMEPTR_NONE]] = {{{.*}}"frame-pointer"="none"{{.*}}}
+// CHECK-O2: attributes #[[OPTNONE_FRAMEPTR_NONE]] = {{{.*}}optnone{{.*}}"frame-pointer"="none"{{.*}}}
+
+// CHECK-O0-NOT: attributes #[[NO_OPTNONE]] = {{{.*}}optnone{{.*}}}
+
+// CHECK-Os: attributes #[[FRAMEPTR_NONLEAF]] = {{{.*}}"frame-pointer"="non-leaf"{{.*}}}
+// CHECK-Os-NOT: attributes #[[FRAMEPTR_NONLEAF]] = {{{.*}}optsize{{.*}}"frame-pointer"="non-leaf"{{.*}}}
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -10220,6 +10220,7 @@
 AddRangeBasedOptnone(NewFD);
 AddImplicitMSFunctionNoBuiltinAttr(NewFD);
 AddSectionMSAllocText(NewFD);