[PATCH] Pragma optimize on/off
Aaron Ballman
aaron at aaronballman.com
Wed May 7 07:10:25 PDT 2014
Some fairly minor nits below.
> Index: lib/Sema/SemaDecl.cpp
> ===================================================================
> --- lib/Sema/SemaDecl.cpp (revision 208197)
> +++ lib/Sema/SemaDecl.cpp (working copy)
> @@ -7378,6 +7378,11 @@
> // marking the function.
> AddCFAuditedAttribute(NewFD);
>
> + // If this is a function definition, check if we have to apply optnone due to
> + // a pragma.
> + if(D.isFunctionDefinition())
> + AddRangeBasedOptnone(NewFD);
> +
> // If this is the first declaration of an extern C variable, update
> // the map of such variables.
> if (NewFD->isFirstDecl() && !NewFD->isInvalidDecl() &&
> Index: lib/Sema/SemaAttr.cpp
> ===================================================================
> --- lib/Sema/SemaAttr.cpp (revision 208197)
> +++ lib/Sema/SemaAttr.cpp (working copy)
> @@ -470,6 +470,34 @@
> D->addAttr(CFAuditedTransferAttr::CreateImplicit(Context, Loc));
> }
>
> +void Sema::ActOnPragmaOptimize(bool On, SourceLocation PragmaLoc) {
> + if(On)
> + OptimizeOffPragmaLocation = SourceLocation();
> + else
> + OptimizeOffPragmaLocation = PragmaLoc;
> +}
> +
> +void Sema::AddRangeBasedOptnone(FunctionDecl *FD) {
> + // In the future, check other pragmas if they're implemented (e.g. pragma
> + // optimize 0 will probably map to this functionality too).
> + if(OptimizeOffPragmaLocation.isValid())
> + AddOptnoneAttributeIfNoConflicts(FD, OptimizeOffPragmaLocation);
> +}
> +
> +void Sema::AddOptnoneAttributeIfNoConflicts(FunctionDecl *FD,
> + SourceLocation Loc) {
> + // Don't add a conflicting attribute. No diagnostic is needed.
> + if (FD->hasAttr<MinSizeAttr>() || FD->hasAttr<AlwaysInlineAttr>())
> + return;
> +
> + // Add attributes only if required. Optnone requires noinline as well, but if
> + // either is already present then don't bother adding them.
> + if (!FD->hasAttr<OptimizeNoneAttr>())
> + FD->addAttr(OptimizeNoneAttr::CreateImplicit(Context, Loc));
> + if (!FD->hasAttr<NoInlineAttr>())
> + FD->addAttr(NoInlineAttr::CreateImplicit(Context, Loc));
> +}
> +
> typedef std::vector<std::pair<unsigned, SourceLocation> > VisStack;
> enum : unsigned { NoVisibility = ~0U };
>
> Index: lib/Parse/ParsePragma.cpp
> ===================================================================
> --- lib/Parse/ParsePragma.cpp (revision 208197)
> +++ lib/Parse/ParsePragma.cpp (working copy)
> @@ -131,6 +131,16 @@
> Token &FirstToken) override;
> };
>
> +/// PragmaOptimizeHandler - "\#pragma clang optimize on/off".
> +struct PragmaOptimizeHandler : public PragmaHandler {
> + PragmaOptimizeHandler(Sema &S)
> + : PragmaHandler("optimize"), Actions(S) {}
> + void HandlePragma(Preprocessor &PP, PragmaIntroducerKind Introducer,
> + Token &FirstToken) override;
> +private:
> + Sema &Actions;
> +};
> +
> } // end namespace
>
> void Parser::initializePragmaHandlers() {
> @@ -195,6 +205,9 @@
> MSSection.reset(new PragmaMSPragma("section"));
> PP.AddPragmaHandler(MSSection.get());
> }
> +
> + OptimizeHandler.reset(new PragmaOptimizeHandler(Actions));
> + PP.AddPragmaHandler("clang", OptimizeHandler.get());
> }
>
> void Parser::resetPragmaHandlers() {
> @@ -249,6 +262,9 @@
>
> PP.RemovePragmaHandler("STDC", FPContractHandler.get());
> FPContractHandler.reset();
> +
> + PP.RemovePragmaHandler("clang", OptimizeHandler.get());
> + OptimizeHandler.reset();
> }
>
> /// \brief Handle the annotation token produced for #pragma unused(...)
> @@ -1531,3 +1547,35 @@
>
> Actions.ActOnPragmaMSComment(Kind, ArgumentString);
> }
> +
> +// #pragma clang optimize off
> +// #pragma clang optimize on
> +void PragmaOptimizeHandler::HandlePragma(Preprocessor &PP,
> + PragmaIntroducerKind Introducer,
> + Token &FirstToken) {
> + Token Tok;
> + PP.Lex(Tok);
> + if (Tok.isNot(tok::identifier)) {
> + PP.Diag(Tok.getLocation(), diag::err_pragma_optimize_malformed);
> + return;
> + }
> + const IdentifierInfo *II = Tok.getIdentifierInfo();
> + // The only accepted values are 'on' or 'off'.
> + bool IsOn = false;
> + if (II->isStr("on")) {
> + IsOn = true;
> + }
> + else if (!II->isStr("off")) {
Style change -- else if should be on the same line as }
> + PP.Diag(Tok.getLocation(), diag::err_pragma_optimize_malformed);
> + return;
> + }
> + PP.Lex(Tok);
> +
> + if (Tok.isNot(tok::eod)) {
> + PP.Diag(Tok.getLocation(), diag::warn_pragma_extra_tokens_at_eol)
> + << "clang optimize";
> + return;
> + }
> +
> + Actions.ActOnPragmaOptimize(IsOn, FirstToken.getLocation());
> +}
> Index: test/SemaCXX/pragma-optimize.cpp
> ===================================================================
> --- test/SemaCXX/pragma-optimize.cpp (revision 0)
> +++ test/SemaCXX/pragma-optimize.cpp (revision 0)
> @@ -0,0 +1,113 @@
> +// RUN: %clang_cc1 -x c++ -std=c++11 -emit-llvm -O2 < %s | FileCheck %s
> +
> +#pragma clang optimize off
> +
> +// This is a macro definition and therefore its text is not present after
> +// preprocessing. The pragma has no effect here.
> +#define CREATE_FUNC(name) \
> +int name (int param) { \
> + return param; \
> +} \
> +
> +// This is a declaration and therefore it is not decorated with `optnone`.
> +extern int foo(int a, int b);
> +// CHECK-DAG: @_Z3fooii{{.*}} [[ATTRFOO:#[0-9]+]]
> +
> +// This is a definition and therefore it will be decorated with `optnone`.
> +int bar(int x, int y) {
> + for(int i = 0; i < x; ++i)
> + y += x;
> + return y + foo(x, y);
> +}
> +// CHECK-DAG: @_Z3barii{{.*}} [[ATTRBAR:#[0-9]+]]
> +
> +// The function "int created (int param)" created by the macro invocation
> +// is also decorated with the `optnone` attribute because it is within a
> +// region of code affected by the functionality (not because of the position
> +// of the macro definition).
> +CREATE_FUNC (created)
> +// CHECK-DAG: @_Z7createdi{{.*}} [[ATTRCREATED:#[0-9]+]]
> +
> +class MyClass {
> + public:
> + // The declaration of the method is not decorated with `optnone`.
> + int method(int blah);
> +};
> +
> +// The definition of the method instead is decorated with `optnone`.
> +int MyClass::method(int blah) {
> + return blah + 1;
> +}
> +// CHECK-DAG: @_ZN7MyClass6methodEi{{.*}} [[ATTRMETHOD:#[0-9]+]]
> +
> +// A template declaration will not be decorated with `optnone`.
> +template <typename T> T twice (T param);
> +
> +// The template definition will be decorated with the attribute `optnone`.
> +template <typename T> T thrice (T param) {
> + return 3 * param;
> +}
> +
> +// This function definition will not be decorated with `optnone` because the
> +// attribute would conflict with `always_inline`.
> +int __attribute__((always_inline)) baz(int z) {
> + return foo(z, 2);
> +}
> +// CHECK-DAG: @_Z3bazi{{.*}} [[ATTRBAZ:#[0-9]+]]
> +
> +#pragma clang optimize on
> +
> +// The function "int wombat(int param)" created by the macro is not
> +// decorated with `optnone`, because the pragma applies its effects only
> +// after preprocessing. The position of the macro definition is not
> +// relevant.
> +CREATE_FUNC (wombat)
> +// CHECK-DAG: @_Z6wombati{{.*}} [[ATTRWOMBAT:#[0-9]+]]
> +
> +// This instantiation of the "twice" template function with a "float" type
> +// will not have an `optnone` attribute because the template declaration was
> +// not affected by the pragma.
> +float container (float par) {
> + return twice(par);
> +}
> +// CHECK-DAG: @_Z9containerf{{.*}} [[ATTRCONTAINER:#[0-9]+]]
> +// CHECK-DAG: @_Z5twiceIfET_S0_{{.*}} [[ATTRTWICE:#[0-9]+]]
> +
> +// This instantiation of the "thrice" template function with a "float" type
> +// will have an `optnone` attribute because the template definition was
> +// affected by the pragma.
> +float container2 (float par) {
> + return thrice(par);
> +}
> +// CHECK-DAG: @_Z10container2f{{.*}} [[ATTRCONTAINER2:#[0-9]+]]
> +// CHECK-DAG: @_Z6thriceIfET_S0_{{.*}} [[ATTRTHRICEFLOAT:#[0-9]+]]
> +
> +
> +// A template specialization is a new definition and it will not be
> +// decorated with an `optnone` attribute because it is now outside of the
> +// affected region.
> +template<> int thrice(int par) {
> + return (par << 1) + par;
> +}
> +int container3 (int par) {
> + return thrice(par);
> +}
> +// CHECK-DAG: @_Z10container3i{{.*}} [[ATTRCONTAINER3:#[0-9]+]]
> +// CHECK-DAG: @_Z6thriceIiET_S0_{{.*}} [[ATTRTHRICEINT:#[0-9]+]]
> +
> +
> +// Check for both noinline and optnone on each function that should have them.
> +// CHECK-DAG: attributes [[ATTRBAR]] = { {{.*}}noinline{{.*}}optnone{{.*}} }
> +// CHECK-DAG: attributes [[ATTRCREATED]] = { {{.*}}noinline{{.*}}optnone{{.*}} }
> +// CHECK-DAG: attributes [[ATTRMETHOD]] = { {{.*}}noinline{{.*}}optnone{{.*}} }
> +// CHECK-DAG: attributes [[ATTRTHRICEFLOAT]] = { {{.*}}noinline{{.*}}optnone{{.*}} }
> +
> +// Check that the other functions do NOT have optnone.
> +// CHECK-DAG-NOT: attributes [[ATTRFOO]] = { {{.*}}optnone{{.*}} }
> +// CHECK-DAG-NOT: attributes [[ATTRBAZ]] = { {{.*}}optnone{{.*}} }
> +// CHECK-DAG-NOT: attributes [[ATTRWOMBAT]] = { {{.*}}optnone{{.*}} }
> +// CHECK-DAG-NOT: attributes [[ATTRCONTAINER]] = { {{.*}}optnone{{.*}} }
> +// CHECK-DAG-NOT: attributes [[ATTRTWICE]] = { {{.*}}optnone{{.*}} }
> +// CHECK-DAG-NOT: attributes [[ATTRCONTAINER2]] = { {{.*}}optnone{{.*}} }
> +// CHECK-DAG-NOT: attributes [[ATTRCONTAINER3]] = { {{.*}}optnone{{.*}} }
> +// CHECK-DAG-NOT: attributes [[ATTRTHRICEINT]] = { {{.*}}optnone{{.*}} }
> Index: test/SemaCXX/pragma-optimize-diagnostics.cpp
> ===================================================================
> --- test/SemaCXX/pragma-optimize-diagnostics.cpp (revision 0)
> +++ test/SemaCXX/pragma-optimize-diagnostics.cpp (revision 0)
This seems like it should be a parser test more than a sema test.
> @@ -0,0 +1,19 @@
> +// RUN: %clang_cc1 -fsyntax-only -verify %s
> +
> +#pragma clang optimize off
> +
> +#pragma clang optimize on
> +
> +#pragma clang optimize on top of spaghetti // expected-warning {{extra tokens at end of '#pragma clang optimize' - ignored}}
> +
> +#pragma clang optimize something_wrong // expected-error {{malformed argument to '#pragma clang optimize' - expected 'on' or 'off'}}
> +
> +#pragma clang optimize // expected-error {{malformed argument to '#pragma clang optimize' - expected 'on' or 'off'}}
> +
> +#define OFF off
> +#define ON on
> +
> +#pragma clang optimize OFF
> +
> +#pragma clang optimize ON
> +
> Index: include/clang/Parse/Parser.h
> ===================================================================
> --- include/clang/Parse/Parser.h (revision 208197)
> +++ include/clang/Parse/Parser.h (working copy)
> @@ -160,6 +160,7 @@
> std::unique_ptr<PragmaHandler> MSConstSeg;
> std::unique_ptr<PragmaHandler> MSCodeSeg;
> std::unique_ptr<PragmaHandler> MSSection;
> + std::unique_ptr<PragmaHandler> OptimizeHandler;
>
> std::unique_ptr<CommentHandler> CommentSemaHandler;
>
> Index: include/clang/Basic/DiagnosticParseKinds.td
> ===================================================================
> --- include/clang/Basic/DiagnosticParseKinds.td (revision 208197)
> +++ include/clang/Basic/DiagnosticParseKinds.td (working copy)
> @@ -841,6 +841,9 @@
> def err_pragma_pointers_to_members_unknown_kind : Error<
> "unexpected %0, expected to see one of %select{|'best_case', 'full_generality', }1"
> "'single_inheritance', 'multiple_inheritance', or 'virtual_inheritance'">;
> +// - #pragma clang optimize on/off
> +def err_pragma_optimize_malformed : Error<
> + "malformed argument to '#pragma clang optimize' - expected 'on' or 'off'">;
Should probably be worded to use "unexpected" instead of "malformed."
Perhaps: "unexpected argument '%0'; expected 'on' or 'off'" or
something akin to that?
>
> // OpenCL Section 6.8.g
> def err_not_opencl_storage_class_specifier : Error<
> Index: include/clang/Sema/Sema.h
> ===================================================================
> --- include/clang/Sema/Sema.h (revision 208197)
> +++ include/clang/Sema/Sema.h (working copy)
> @@ -333,6 +333,11 @@
> /// VisContext - Manages the stack for \#pragma GCC visibility.
> void *VisContext; // Really a "PragmaVisStack*"
>
> + /// \brief This represents the last location of a "#pragma clang optimize off"
> + /// directive if such directive has not been closed by an "on" yet. If
"such directive" should be "such a directive".
> + /// optimizations are currently "on", this is set to an invalid location.
> + SourceLocation OptimizeOffPragmaLocation;
> +
> /// \brief Flag indicating if Sema is building a recovery call expression.
> ///
> /// This flag is used to avoid building recovery call expressions
> @@ -7228,6 +7233,19 @@
> /// the appropriate attribute.
> void AddCFAuditedAttribute(Decl *D);
>
> + /// \brief Called on well formed \#pragma clang optimize.
> + void ActOnPragmaOptimize(bool On, SourceLocation PragmaLoc);
> +
> + /// \brief Only called on function definitions; if there is a pragma in scope
> + /// with the effect of a range-based optnone, consider marking the function
> + /// with attribute optnone.
> + void AddRangeBasedOptnone(FunctionDecl *FD);
> +
> + /// \brief Adds the 'optnone' attribute to the function declaration if there
> + /// are no conflicts; Loc represents the location causing the 'optnone'
> + /// attribute to be added (usually because of a pragma).
> + void AddOptnoneAttributeIfNoConflicts(FunctionDecl *FD, SourceLocation Loc);
> +
> /// AddAlignedAttr - Adds an aligned attribute to a particular declaration.
> void AddAlignedAttr(SourceRange AttrRange, Decl *D, Expr *E,
> unsigned SpellingListIndex, bool IsPackExpansion);
>
~Aaron
On Wed, May 7, 2014 at 9:24 AM, Dario Domizioli
<dario.domizioli at gmail.com> wrote:
> Hi Richard.
> Thanks for the quick reply and for the review!
>
> I am attaching a new patch which fixes the style issues and adds a specific
> test case for all the expected diagnostics coming from the handling of
> '#pragma clang optimize'.
>
>
> On 6 May 2014 22:26, Richard Smith <richard at metafoo.co.uk> wrote:
>>
>> What should happen if a preprocessed header or a TU preamble ends with a
>> "#pragma clang optimize off" directive active? For a module, I don't think
>> it should have any effect, but in those other two cases I think we should
>> reactivate the directive when loading the preamble or PCH.
>
>
> Yes, if we reach the end of a compilation unit and the state of the pragma
> is still "off" then nothing special happens. This is because users may want
> to disable optimizations in a whole file by just having '#pragma clang
> optimize off' at the top of the file, and they shouldn't need a matching
> "on" at the end.
> However, you make a good point about PCH. I haven't looked at this in
> detail, but I suppose we would have to somehow serialize the state of
> OptimizeOffPragmaLocation in the precompiled header. This should be enough,
> because any function definition in the header (if any is present) would have
> been decorated with 'optnone' already, so at the point of serialization the
> effect of the pragma has already been applied and we just serialize the
> attributes as usual.
>
> At the moment the patch does not include serialization in the PCH, but I
> will have a look at how it's done and implement that too.
> Would you prefer to have just one patch with everything in it, or would you
> be OK with getting this in and then incrementally improve it with another
> patch?
>
>
>>
>> + PP.Lex(Tok);
>>
>> Should we perform macro expansion on this token or not?
>
>
> Apparently, by the time the token gets there it has already been expanded.
> I have verified this in the additional test case, with:
> #define OFF off
> #define ON on
> #pragma clang optimize OFF
> #pragma clang optimize ON
> It does not seem to produce any diagnostic, so the tokens are processed as
> 'off' and 'on'.
>
> If you are OK with the updated patch, could you please commit it for me? (I
> don't have commit access yet)
>
> Cheers,
> Dario Domizioli
> SN Systems - Sony Computer Entertainment Group
>
>
>
>
>
>
>
>>
>>
>>
>> + if (II->isStr("on")) {
>> + isOn = true;
>> + PP.Lex(Tok);
>> + }
>> + else if (II->isStr("off"))
>> + PP.Lex(Tok);
>> + else {
>> + PP.Diag(Tok.getLocation(), diag::err_pragma_optimize_malformed);
>> + return;
>> + }
>>
>> Please sink the PP.Lex call below this if/else block. Also, you don't seem
>> to have any test coverage for the 'malformed' case.
>>
>>
>> + /// OptimizeOffPragmaLocation - it represents the last location of a
>> "#pragma
>> + /// clang optimize off" directive if such directive has not been closed
>> by an
>> + /// "on" yet. If optimizations are currently "on", this is set to an
>> invalid
>> + /// location.
>> + SourceLocation OptimizeOffPragmaLocation;
>>
>> Style: Please start this comment with "\brief" instead of the name of the
>> entity being declared. (Likewise for the next three declarations.)
>>
>>
>> On Tue, May 6, 2014 at 6:38 AM, Dario Domizioli
>> <dario.domizioli at gmail.com> wrote:
>>>
>>> Hi,
>>>
>>> Please review the attached patch for adding "pragma clang optimize
>>> on/off" to clang.
>>> This pragma is implemented by decorating function definitions in the
>>> "off" regions with attribute 'optnone'.
>>>
>>> The syntax and semantics have been discussed in here:
>>> http://lists.cs.uiuc.edu/pipermail/cfe-dev/2014-April/036561.html
>>> The final consensus was that the feature was worth having, and after a
>>> while I think we converged on the syntax, so I am now sending the patch for
>>> review.
>>>
>>> Here is a very short summary of how the pragma works:
>>>
>>> // The default status is "on".
>>> #pragma clang optimize off
>>> // Function definitions in here are decorated with attribute
>>> 'optnone'
>>> // but only if it does not conflict with existing attributes.
>>> #pragma clang optimize on
>>> // Code compiled as normal.
>>>
>>> The test provided with the patch checks a number of interesting cases,
>>> including template definitions and instantiations, macros, and conflicting
>>> attributes (e.g. always_inline).
>>>
>>> I will prepare another patch for the documentation.
>>>
>>> Cheers,
>>> Dario Domizioli
>>> SN Systems - Sony Computer Entertainment Group
>>>
>>>
>>>
>>> _______________________________________________
>>> cfe-commits mailing list
>>> cfe-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>
>>
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
More information about the cfe-commits
mailing list