This is an archive of the discontinued LLVM Phabricator instance.

[MSVC] Add initial support for MSVC pragma optimize
ClosedPublic

Authored by steplong on May 16 2022, 1:05 PM.

Details

Summary

MSVC's pragma optimize turns optimizations on or off based on the list
passed. At the moment, we only support an empty optimization list.

i.e. #pragma optimize("", on | off)

From MSVC's docs:

ParameterType of optimization
gEnable global optimizations. Deprecated
s or tSpecify short or fast sequences of machine code
yGenerate frame pointers on the program stack

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

Diff Detail

Event Timeline

steplong created this revision.May 16 2022, 1:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 16 2022, 1:05 PM
steplong requested review of this revision.May 16 2022, 1:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 16 2022, 1:05 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
steplong edited the summary of this revision. (Show Details)May 16 2022, 1:11 PM
aaron.ballman added inline comments.May 31 2022, 8:15 AM
clang/lib/Parse/ParsePragma.cpp
3653–3654

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

3654
clang/lib/Sema/SemaAttr.cpp
1147

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

1156

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

Can you explain why this is needed?

aaron.ballman added inline comments.May 31 2022, 8:19 AM
clang/lib/Sema/SemaAttr.cpp
1226–1232

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.

steplong added inline comments.May 31 2022, 9:32 AM
clang/lib/Sema/SemaAttr.cpp
1226–1232

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

1243

I added this attr, so we can remove the attr optnone even if -O0 was passed on the cmdline

rnk added a comment.May 31 2022, 10:26 AM

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.

clang/lib/Parse/ParsePragma.cpp
3698

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

aaron.ballman added inline comments.May 31 2022, 11:53 AM
clang/lib/Sema/SemaAttr.cpp
1226–1232

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.

1243

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.

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

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.

rnk added a comment.Jun 2 2022, 9:47 AM

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.

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.

steplong updated this revision to Diff 436476.Jun 13 2022, 10:57 AM
  • Parse the string in the pragma
  • Some of the tests should fail. I'll fix them up once D126984 looks good
steplong edited the summary of this revision. (Show Details)Jun 13 2022, 10:58 AM
steplong added inline comments.
clang/include/clang/Sema/Sema.h
768

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

xbolva00 added a subscriber: xbolva00.EditedJun 14 2022, 8:15 AM

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.

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.

steplong updated this revision to Diff 436954.Jun 14 2022, 2:51 PM
steplong edited the summary of this revision. (Show Details)
  • Change logic to only handle empty optimization list
  • Fixup doc and tests
steplong retitled this revision from [MSVC] Add support for MSVC pragma optimize to [MSVC] Add initial support for MSVC pragma optimize.Jun 14 2022, 2:52 PM
steplong edited the summary of this revision. (Show Details)
aaron.ballman added inline comments.Jun 15 2022, 6:15 AM
clang/docs/ReleaseNotes.rst
339
clang/include/clang/Sema/Sema.h
761–762

I think we can get rid of this since it's unused now.

clang/lib/Sema/SemaAttr.cpp
1190

I think this should also be removed, as we never assign anything into that variable anyway.

1192–1197
steplong updated this revision to Diff 437153.Jun 15 2022, 7:14 AM
  • Fix docs
  • Clean up code
aaron.ballman accepted this revision.Jun 15 2022, 9:00 AM

LGTM! But given the number of people who've expressed opinions on this, please wait a day or two before landing in case other reviewers have additional feedback.

This revision is now accepted and ready to land.Jun 15 2022, 9:00 AM
xbolva00 added inline comments.Jun 15 2022, 9:11 AM
clang/docs/ReleaseNotes.rst
342

So to sum up.. #pragma optimize("", off) disables all optimizations, right?

#pragma optimize("", on) enables all, right? is no op if compiled with -O0?

Other than this release note, looks like there is no way to specifiy own documentation for pragmas? :/

Especially if there are differences between MSVC and current implementation...

LGTM! But given the number of people who've expressed opinions on this, please wait a day or two before landing in case other reviewers have additional feedback.

In term of code, implementation ok, some concerns related to documentation added.

LGTM! But given the number of people who've expressed opinions on this, please wait a day or two before landing in case other reviewers have additional feedback.

In term of code, implementation ok, some concerns related to documentation added.

Good call! We could add documentation for this pragma to https://github.com/llvm/llvm-project/blob/main/clang/docs/LanguageExtensions.rst and document the differences there.

xbolva00 added inline comments.Jun 15 2022, 9:26 AM
clang/docs/ReleaseNotes.rst
342

Plus, maybe we should emit a warning if #pragma optimize("", on) is used with -O0?

steplong added inline comments.Jun 15 2022, 9:34 AM
clang/docs/ReleaseNotes.rst
342

Hmm, MSVC says When you use the on parameter, it resets the optimizations to the ones that you specified using the /O compiler option., so I don't think we need to emit a warning. I think we differ from MSVC for off. For MSVC, off turns off frame pointers, "s", and "t". We are just mapping that to optnone.

steplong updated this revision to Diff 437260.Jun 15 2022, 11:18 AM
  • Add documentation on difference btwn MSVC and Clang's implementation of pragma optimize

Thanks

clang/docs/LanguageExtensions.rst
3820

as the clang pragma?

steplong added inline comments.Jun 15 2022, 11:57 AM
clang/docs/LanguageExtensions.rst
3820

The clang pragma #pragma clang optimize off. It's mentioned in this section, but hmm maybe I can make it more clear

steplong updated this revision to Diff 437280.Jun 15 2022, 12:00 PM
  • Fix docs

Hi, I'll land this on Friday. Let me know if there are any comments. Also, I'm not sure how to rebase it on top of main. At the moment, it's still on top of D126984 (i.e. stack has one open)

aaron.ballman added inline comments.Jun 22 2022, 10:08 AM
clang/docs/LanguageExtensions.rst
3818
3819–3820

Would this be more clear?

steplong updated this revision to Diff 439097.Jun 22 2022, 10:58 AM
  • Fix up docs
steplong updated this revision to Diff 439738.Jun 24 2022, 7:08 AM
  • Run pre-merge checks again after abandoning dependent patch D126984
This revision was landed with ongoing or failed builds.Jun 24 2022, 8:04 AM
This revision was automatically updated to reflect the committed changes.