[PATCH] #pragma vectorize
Aaron Ballman
aaron at aaronballman.com
Wed May 7 06:57:04 PDT 2014
Some further comments below (I tried to not echo what Richard and Ben
already pointed out, but do agree with their review).
> Index: include/clang/AST/Attr.h
> ===================================================================
> --- include/clang/AST/Attr.h (revision 208124)
> +++ include/clang/AST/Attr.h (working copy)
> @@ -16,6 +16,7 @@
>
> #include "clang/AST/AttrIterator.h"
> #include "clang/AST/Decl.h"
> +#include "clang/AST/Expr.h"
Is there a reason this include is required in Attr.h?
> #include "clang/AST/Type.h"
> #include "clang/Basic/AttrKinds.h"
> #include "clang/Basic/LLVM.h"
> Index: include/clang/Basic/Attr.td
> ===================================================================
> --- include/clang/Basic/Attr.td (revision 208124)
> +++ include/clang/Basic/Attr.td (working copy)
> @@ -1750,3 +1750,50 @@
> def Unaligned : IgnoredAttr {
> let Spellings = [Keyword<"__unaligned">];
> }
> +
> +def LoopHint : InheritableAttr {
> + // This attribute has no spellings. It is
> + // created when pragma loop is specified.
> + let Spellings = [Keyword<"vectorize">,
> + Keyword<"interleave">];
> +
> + let Args = [IntArgument<"type">,
> + ExprArgument<"value">];
These strings should be capitalized as they generate code.
> +
> + let AdditionalMembers = [{
> + enum Type {
> + Unknown = 0,
> + Enable = 1,
> + Disable = 2,
> + Value = 4
> + };
> +
> + static bool isCompatible(unsigned State) {
> + return State != (Disable | Enable) &&
> + State != (Disable | Value) &&
> + State != (Disable | Enable | Value);
> + }
> +
> + static StringRef getName(unsigned State) {
> + if (State == Enable) return "enable";
> + else if (State == Disable) return "disable";
> + else if (State == Value) return "value";
> + else if (State == (Enable|Value)) return "enable+value";
> + return "unknown";
> + }
> +
> + void printPragma(raw_ostream &OS, const PrintingPolicy &Policy) const {
> + StringRef Option = getSpelling();
> +
> + if (getType() == Value) {
> + OS << "#pragma loop " << Option << "(";
> + getValue()->printPretty(OS, 0, Policy);
> + OS << ")\n";
> + } else {
> + OS << "#pragma loop " << Option << "(" << getName(getType()) << ")\n";
> + }
> + }
> + }];
> +
> + let Documentation = [Undocumented];
I think this should be documented, but I can understand not doing it
as part of the attribute documentation. As Richard suggested, if we
had a Pragma spelling, that would be beneficial here as well (we could
generate a pragma-specific section of the documentation).
> +}
> Index: include/clang/Basic/DiagnosticGroups.td
> ===================================================================
> --- include/clang/Basic/DiagnosticGroups.td (revision 208124)
> +++ include/clang/Basic/DiagnosticGroups.td (working copy)
> @@ -688,3 +688,6 @@
>
> // Instrumentation based profiling warnings.
> def ProfileInstrOutOfDate : DiagGroup<"profile-instr-out-of-date">;
> +
> +// #pragma loop directive based warnings.
> +def PragmaLoop : DiagGroup<"pragma-loop">;
> Index: include/clang/Basic/DiagnosticParseKinds.td
> ===================================================================
> --- include/clang/Basic/DiagnosticParseKinds.td (revision 208124)
> +++ include/clang/Basic/DiagnosticParseKinds.td (working copy)
> @@ -882,6 +882,20 @@
> "unexpected OpenMP clause '%0' in directive '#pragma omp %1'">;
> def err_omp_more_one_clause : Error<
> "directive '#pragma omp %0' cannot contain more than one '%1' clause">;
> +
> +// Pragma loop support.
> +def warn_pragma_loop_invalid_option : Warning<
> + "invalid option '%0' in directive '#pragma loop', expected either vectorize or interleave - ignoring">,
> + InGroup<PragmaLoop>;
> +def warn_pragma_loop_missing_option : Warning<
> + "missing option in directive '#pragma loop', expected either vectorize or interleave - ignoring">,
> + InGroup<PragmaLoop>;
These two could be combined into:
"%select{missing|invalid}0 option %select{|'%1'}0 in directive
'#pragma loop', expected either vectorize or interleave - ignoring"
> +def warn_pragma_loop_invalid_type : Warning<
> + "invalid value '%0' in directive '#pragma loop %1', expected either 'enable', 'disable', or a positive integer - ignoring">,
> + InGroup<PragmaLoop>;
> +def warn_pragma_loop_precedes_nonloop : Warning<
> + "expected a for, while, or do-while loop to follow the '#pragma loop' directive - ignoring">,
> + InGroup<PragmaLoop>;
> } // end of Parse Issue category.
>
> let CategoryName = "Modules Issue" in {
> Index: include/clang/Basic/DiagnosticSemaKinds.td
> ===================================================================
> --- include/clang/Basic/DiagnosticSemaKinds.td (revision 208124)
> +++ include/clang/Basic/DiagnosticSemaKinds.td (working copy)
> @@ -537,6 +537,12 @@
> "#pragma visibility pop with no matching #pragma visibility push">;
> def note_surrounding_namespace_starts_here : Note<
> "surrounding namespace with visibility attribute starts here">;
> +def warn_pragma_loop_invalid_value : Warning<
> + "expected a positive integer in directive '#pragma loop %0' - ignoring">,
> + InGroup<PragmaLoop>;
> +def warn_pragma_loop_incompatible : Warning<
> + "'%0' and '%1' directive option types are incompatible in '#pragma loop %2' - ignoring">,
> + InGroup<PragmaLoop>;
>
> /// Objective-C parser diagnostics
> def err_duplicate_class_def : Error<
> Index: include/clang/Basic/LoopHint.h
> ===================================================================
> --- include/clang/Basic/LoopHint.h (revision 0)
> +++ include/clang/Basic/LoopHint.h (working copy)
> @@ -0,0 +1,32 @@
> +//===--- LoopHint.h - Types for LoopHint ------------------------*- C++ -*-===//
> +//
> +// The LLVM Compiler Infrastructure
> +//
> +// This file is distributed under the University of Illinois Open Source
> +// License. See LICENSE.TXT for details.
> +//
> +//===----------------------------------------------------------------------===//
> +
> +
> +#ifndef LLVM_CLANG_BASIC_LOOPHINT_H
> +#define LLVM_CLANG_BASIC_LOOPHINT_H
> +
> +#include "clang/Basic/IdentifierTable.h"
> +#include "clang/Basic/SourceLocation.h"
> +#include "clang/Sema/AttributeList.h"
> +#include "clang/Sema/Ownership.h"
This is a layering violation (to include things from Sema into Basic).
> +
> +namespace clang {
> +
> +/// \brief Loop hint specified by a pragma loop directive
> +/// and used by codegen to attach metadata to the IR.
> +struct LoopHint {
> + SourceRange Range;
> + ExprResult ValueExpr;
> + IdentifierLoc *ValueLoc;
> + IdentifierLoc *OptionLoc;
> +};
> +
> +} // end namespace clang
> +
> +#endif // LLVM_CLANG_BASIC_LOOPHINT_H
> Index: include/clang/Basic/TokenKinds.def
> ===================================================================
> --- include/clang/Basic/TokenKinds.def (revision 208124)
> +++ include/clang/Basic/TokenKinds.def (working copy)
> @@ -701,6 +701,11 @@
> ANNOTATION(pragma_openmp)
> ANNOTATION(pragma_openmp_end)
>
> +// Annotations for loop pragma directives #pragma loop ...
> +// The lexer produces these so that they only take effect when the parser
> +// handles #pragma loop ... directives.
> +ANNOTATION(pragma_loop_hint)
> +
> // Annotations for module import translated from #include etc.
> ANNOTATION(module_include)
> ANNOTATION(module_begin)
> Index: include/clang/Parse/Parser.h
> ===================================================================
> --- include/clang/Parse/Parser.h (revision 208124)
> +++ include/clang/Parse/Parser.h (working copy)
> @@ -14,6 +14,7 @@
> #ifndef LLVM_CLANG_PARSE_PARSER_H
> #define LLVM_CLANG_PARSE_PARSER_H
>
> +#include "clang/Basic/LoopHint.h"
> #include "clang/Basic/OpenMPKinds.h"
> #include "clang/Basic/OperatorPrecedence.h"
> #include "clang/Basic/Specifiers.h"
> @@ -160,6 +161,7 @@
> std::unique_ptr<PragmaHandler> MSConstSeg;
> std::unique_ptr<PragmaHandler> MSCodeSeg;
> std::unique_ptr<PragmaHandler> MSSection;
> + std::unique_ptr<PragmaHandler> LoopHintHandler;
>
> std::unique_ptr<CommentHandler> CommentSemaHandler;
>
> @@ -518,6 +520,10 @@
> /// #pragma clang __debug captured
> StmtResult HandlePragmaCaptured();
>
> + /// \brief Handle the annotation token produced for
> + /// #pragma vectorize...
> + LoopHint HandlePragmaLoopHint();
> +
> /// GetLookAheadToken - This peeks ahead N tokens and returns that token
> /// without consuming any tokens. LookAhead(0) returns 'Tok', LookAhead(1)
> /// returns the token after Tok, etc.
> @@ -1600,6 +1606,10 @@
> StmtResult ParseReturnStatement();
> StmtResult ParseAsmStatement(bool &msAsm);
> StmtResult ParseMicrosoftAsmStatement(SourceLocation AsmLoc);
> + StmtResult ParsePragmaLoopHint(StmtVector &Stmts,
> + bool OnlyStatement,
> + SourceLocation *TrailingElseLoc,
> + ParsedAttributesWithRange &Attrs);
>
> /// \brief Describes the behavior that should be taken for an __if_exists
> /// block.
> Index: lib/AST/StmtPrinter.cpp
> ===================================================================
> --- lib/AST/StmtPrinter.cpp (revision 208124)
> +++ lib/AST/StmtPrinter.cpp (working copy)
> @@ -168,19 +168,33 @@
> }
>
> void StmtPrinter::VisitAttributedStmt(AttributedStmt *Node) {
> - OS << "[[";
> bool first = true;
> - for (ArrayRef<const Attr*>::iterator it = Node->getAttrs().begin(),
> - end = Node->getAttrs().end();
> - it != end; ++it) {
> - if (!first) {
> - OS << ", ";
> - first = false;
> + std::string raw_attr_os;
> + llvm::raw_string_ostream AttrOS(raw_attr_os);
> + for (ArrayRef<const Attr*>::reverse_iterator it = Node->getAttrs().rbegin(),
> + end = Node->getAttrs().rend();
> + it != end; ++it) {
> + // FIXME: When we get more attributes which have a pragma
> + // form LoopHintAttr should be replaced with a base class
> + // for pretty printing these types of attributes.
> + if (const LoopHintAttr *LHA = dyn_cast<LoopHintAttr>(*it))
> + LHA->printPragma(OS, Policy);
I'm not too keen on this approach. Everything should be handled
through printPretty, which is generated in ClangAttrEmitter.cpp. This
further suggests needing a Pragma spelling for the attribute.
> + else {
> + if (!first) {
> + AttrOS << ", ";
> + first = false;
> + }
> + // TODO: check this
> + (*it)->printPretty(AttrOS, Policy);
> }
> - // TODO: check this
> - (*it)->printPretty(OS, Policy);
> }
> - OS << "]] ";
> +
> + // Check to see if any attributes were printed
> + StringRef AttrStr = AttrOS.str();
> + if (AttrStr.size() > 0) {
I would prefer this to be !AttrStr.empty() for clarity.
> + OS << "[[" << AttrStr << "]] ";
> + }
> +
> PrintStmt(Node->getSubStmt(), 0);
> }
>
> Index: lib/CodeGen/CGStmt.cpp
> ===================================================================
> --- lib/CodeGen/CGStmt.cpp (revision 208124)
> +++ lib/CodeGen/CGStmt.cpp (working copy)
> @@ -16,6 +16,7 @@
> #include "CodeGenModule.h"
> #include "TargetInfo.h"
> #include "clang/AST/StmtVisitor.h"
> +#include "clang/Basic/LoopHint.h"
> #include "clang/Basic/PrettyStackTrace.h"
> #include "clang/Basic/TargetInfo.h"
> #include "clang/Sema/SemaDiagnostic.h"
> @@ -396,7 +397,23 @@
> }
>
> void CodeGenFunction::EmitAttributedStmt(const AttributedStmt &S) {
> - EmitStmt(S.getSubStmt());
> + const Stmt *SubStmt = S.getSubStmt();
> + switch (SubStmt->getStmtClass()) {
> + case Stmt::DoStmtClass:
> + EmitDoStmt(cast<DoStmt>(*SubStmt), S.getAttrs());
> + break;
> + case Stmt::ForStmtClass:
> + EmitForStmt(cast<ForStmt>(*SubStmt), S.getAttrs());
> + break;
> + case Stmt::WhileStmtClass:
> + EmitWhileStmt(cast<WhileStmt>(*SubStmt), S.getAttrs());
> + break;
> + case Stmt::CXXForRangeStmtClass:
> + EmitCXXForRangeStmt(cast<CXXForRangeStmt>(*SubStmt), S.getAttrs());
> + break;
> + default:
> + EmitStmt(SubStmt);
> + }
> }
>
> void CodeGenFunction::EmitGotoStmt(const GotoStmt &S) {
> @@ -502,7 +519,115 @@
> EmitBlock(ContBlock, true);
> }
>
> -void CodeGenFunction::EmitWhileStmt(const WhileStmt &S) {
> +void CodeGenFunction::EmitCondBrHints(llvm::LLVMContext &Context,
> + llvm::BranchInst *CondBr,
> + ArrayRef<const Attr*> &Attrs) {
> + // Do not continue if there are not hints.
> + if (Attrs.empty())
> + return;
> +
> + unsigned int VectorizeState = 0;
> + unsigned int InterleaveState = 0;
> +
> + // Add vectorize hints to the metadata on the conditional branch.
> + // Iterate in reverse so hints are put in the same order they appear.
> + SmallVector<llvm::Value*, 2> Metadata(1);
> + ArrayRef<const Attr*>::reverse_iterator I;
> + for (I = Attrs.rbegin(); I != Attrs.rend(); ++I) {
Check against rend() should be hoisted (or use a range-based loop with
an adaptor).
> + const LoopHintAttr *LH = cast<LoopHintAttr>(*I);
> +
> + Expr *ValueExpr = LH->getValue();
> + unsigned int Type = LH->getType();
> +
> + int ValueInt = 1;
> + if (Type == LoopHintAttr::Value) {
> + llvm::APSInt ValueAPS;
> + if(!ValueExpr || !ValueExpr->EvaluateAsInt(ValueAPS, CGM.getContext()) ||
> + (ValueInt = ValueAPS.getSExtValue()) < 1) {
> + CGM.getDiags().Report(LH->getRange().getEnd(),
> + diag::warn_pragma_loop_invalid_value) <<
> + LH->getSpelling();
I may be incorrect, but the diagnostics engine should understand how
to report the attribute without resorting to calling getSpelling on
it. At least, that's the way it works in Sema (and CG should behave
the same way if it doesn't already). This comment applies to all
diagnostic reporting in the patch, not just this one.
> + continue;
> + }
> + }
> +
> + llvm::Value *Value;
> + llvm::MDString *Name;
> + LoopHintAttr::Spelling S = LH->getSemanticSpelling();
Instead of relying on the semantic spelling here, this would be more
clear as an AdditionalMember on the attribute that then gets used
here.
> +
> + if (S == LoopHintAttr::Keyword_vectorize) {
> + // Do not add hint if it is incompatible with prior hints.
> + if (!LoopHintAttr::isCompatible(VectorizeState | Type)) {
> + CGM.getDiags().Report(LH->getRange().getEnd(),
> + diag::warn_pragma_loop_incompatible) <<
> + LoopHintAttr::getName(VectorizeState) <<
> + LoopHintAttr::getName(Type) <<
> + LH->getSpelling();
> + continue;
> + }
> +
> + VectorizeState |= Type;
> +
> + switch(Type) {
> + case LoopHintAttr::Enable:
> + Name = llvm::MDString::get(Context, "llvm.vectorizer.enable");
> + Value = Builder.getTrue();
> + break;
> + case LoopHintAttr::Disable:
> + Name = llvm::MDString::get(Context, "llvm.vectorizer.width");
> + Value = llvm::ConstantInt::get(Int32Ty, 1);
> + break;
> + case LoopHintAttr::Value:
> + Name = llvm::MDString::get(Context, "llvm.vectorizer.width");
> + Value = llvm::ConstantInt::get(Int32Ty, ValueInt);
> + break;
> + }
> + } else if (S == LoopHintAttr::Keyword_interleave) {
> + // Do not add hint if it is incompatible with prior hints.
> + if (!LoopHintAttr::isCompatible(InterleaveState | Type)) {
> + CGM.getDiags().Report(LH->getRange().getEnd(),
> + diag::warn_pragma_loop_incompatible) <<
> + LoopHintAttr::getName(InterleaveState) <<
> + LoopHintAttr::getName(Type) <<
> + LH->getSpelling();
> + continue;
> + }
> +
> + InterleaveState |= Type;
> +
> + switch(Type) {
> + case LoopHintAttr::Enable:
> + Name = llvm::MDString::get(Context, "llvm.vectorizer.enable");
> + Value = Builder.getTrue();
> + break;
> + case LoopHintAttr::Disable:
> + Name = llvm::MDString::get(Context, "llvm.vectorizer.unroll");
> + Value = llvm::ConstantInt::get(Int32Ty, 1);
> + break;
> + case LoopHintAttr::Value:
> + Name = llvm::MDString::get(Context, "llvm.vectorizer.unroll");
> + Value = llvm::ConstantInt::get(Int32Ty, ValueInt);
> + break;
> + }
> + }
> +
> + SmallVector<llvm::Value*, 2> OpValues;
> + OpValues.push_back(Name);
> + OpValues.push_back(Value);
> +
> + // Set or overwrite metadata indicated by Name.
> + Metadata.push_back(llvm::MDNode::get(Context, OpValues));
> + }
> +
> + // Add llvm.loop MDNode to CondBr.
> + llvm::MDNode *LoopID = llvm::MDNode::get(Context, Metadata);
> + LoopID->replaceOperandWith(0, LoopID); // First op points to itself.
> +
> + CondBr->setMetadata("llvm.loop", LoopID);
> +}
> +
> +void CodeGenFunction::EmitWhileStmt(const WhileStmt &S,
> + ArrayRef<const Attr*> WhileAttrs) {
> RegionCounter Cnt = getPGORegionCounter(&S);
>
> // Emit the header for the loop, which will also become
> @@ -547,13 +672,17 @@
> llvm::BasicBlock *ExitBlock = LoopExit.getBlock();
> if (ConditionScope.requiresCleanups())
> ExitBlock = createBasicBlock("while.exit");
> - Builder.CreateCondBr(BoolCondVal, LoopBody, ExitBlock,
> - PGO.createLoopWeights(S.getCond(), Cnt));
> + llvm::BranchInst *CondBr = Builder.CreateCondBr(
> + BoolCondVal, LoopBody, ExitBlock,
> + PGO.createLoopWeights(S.getCond(), Cnt));
>
> if (ExitBlock != LoopExit.getBlock()) {
> EmitBlock(ExitBlock);
> EmitBranchThroughCleanup(LoopExit);
> }
> +
> + // Attach metadata to loop body conditional branch
Missing punctuation in the comment (here and elsewhere).
> + EmitCondBrHints(LoopBody->getContext(), CondBr, WhileAttrs);
> }
>
> // Emit the loop body. We have to emit this in a cleanup scope
> @@ -582,7 +711,8 @@
> SimplifyForwardingBlocks(LoopHeader.getBlock());
> }
>
> -void CodeGenFunction::EmitDoStmt(const DoStmt &S) {
> +void CodeGenFunction::EmitDoStmt(const DoStmt &S,
> + ArrayRef<const Attr*> DoAttrs) {
> JumpDest LoopExit = getJumpDestInCurrentScope("do.end");
> JumpDest LoopCond = getJumpDestInCurrentScope("do.cond");
>
> @@ -619,10 +749,15 @@
> EmitBoolCondBranch = false;
>
> // As long as the condition is true, iterate the loop.
> - if (EmitBoolCondBranch)
> - Builder.CreateCondBr(BoolCondVal, LoopBody, LoopExit.getBlock(),
> - PGO.createLoopWeights(S.getCond(), Cnt));
> + if (EmitBoolCondBranch) {
> + llvm::BranchInst *CondBr = Builder.CreateCondBr(
> + BoolCondVal, LoopBody, LoopExit.getBlock(),
> + PGO.createLoopWeights(S.getCond(), Cnt));
>
> + // Attach metadata to loop body conditional branch
> + EmitCondBrHints(LoopBody->getContext(), CondBr, DoAttrs);
> + }
> +
> // Emit the exit block.
> EmitBlock(LoopExit.getBlock());
>
> @@ -632,7 +767,8 @@
> SimplifyForwardingBlocks(LoopCond.getBlock());
> }
>
> -void CodeGenFunction::EmitForStmt(const ForStmt &S) {
> +void CodeGenFunction::EmitForStmt(const ForStmt &S,
> + ArrayRef<const Attr*> ForAttrs) {
> JumpDest LoopExit = getJumpDestInCurrentScope("for.end");
>
> RunCleanupsScope ForScope(*this);
> @@ -686,9 +822,13 @@
> // C99 6.8.5p2/p4: The first substatement is executed if the expression
> // compares unequal to 0. The condition must be a scalar type.
> llvm::Value *BoolCondVal = EvaluateExprAsBool(S.getCond());
> - Builder.CreateCondBr(BoolCondVal, ForBody, ExitBlock,
> - PGO.createLoopWeights(S.getCond(), Cnt));
> + llvm::BranchInst *CondBr = Builder.CreateCondBr(
> + BoolCondVal, ForBody, ExitBlock,
> + PGO.createLoopWeights(S.getCond(), Cnt));
>
> + // Attach metadata to loop body conditional branch
> + EmitCondBrHints(ForBody->getContext(), CondBr, ForAttrs);
> +
> if (ExitBlock != LoopExit.getBlock()) {
> EmitBlock(ExitBlock);
> EmitBranchThroughCleanup(LoopExit);
> @@ -728,7 +868,8 @@
> EmitBlock(LoopExit.getBlock(), true);
> }
>
> -void CodeGenFunction::EmitCXXForRangeStmt(const CXXForRangeStmt &S) {
> +void CodeGenFunction::EmitCXXForRangeStmt(const CXXForRangeStmt &S,
> + ArrayRef<const Attr*> ForAttrs) {
> JumpDest LoopExit = getJumpDestInCurrentScope("for.end");
>
> RunCleanupsScope ForScope(*this);
> @@ -761,9 +902,13 @@
> // The body is executed if the expression, contextually converted
> // to bool, is true.
> llvm::Value *BoolCondVal = EvaluateExprAsBool(S.getCond());
> - Builder.CreateCondBr(BoolCondVal, ForBody, ExitBlock,
> - PGO.createLoopWeights(S.getCond(), Cnt));
> + llvm::BranchInst *CondBr = Builder.CreateCondBr(BoolCondVal, ForBody,
> + ExitBlock,
> + PGO.createLoopWeights(S.getCond(), Cnt));
>
> + // Attach metadata to loop body conditional branch
> + EmitCondBrHints(ForBody->getContext(), CondBr, ForAttrs);
> +
> if (ExitBlock != LoopExit.getBlock()) {
> EmitBlock(ExitBlock);
> EmitBranchThroughCleanup(LoopExit);
> Index: lib/CodeGen/CodeGenFunction.h
> ===================================================================
> --- lib/CodeGen/CodeGenFunction.h (revision 208124)
> +++ lib/CodeGen/CodeGenFunction.h (working copy)
> @@ -1846,9 +1846,16 @@
> void EmitGotoStmt(const GotoStmt &S);
> void EmitIndirectGotoStmt(const IndirectGotoStmt &S);
> void EmitIfStmt(const IfStmt &S);
> - void EmitWhileStmt(const WhileStmt &S);
> - void EmitDoStmt(const DoStmt &S);
> - void EmitForStmt(const ForStmt &S);
> +
> + void EmitCondBrHints(llvm::LLVMContext &Context,
> + llvm::BranchInst *CondBr,
> + ArrayRef<const Attr*> &Attrs);
> + void EmitWhileStmt(const WhileStmt &S,
> + ArrayRef<const Attr*> Attrs = ArrayRef<const Attr*>());
> + void EmitDoStmt(const DoStmt &S,
> + ArrayRef<const Attr*> Attrs = ArrayRef<const Attr*>());
> + void EmitForStmt(const ForStmt &S,
> + ArrayRef<const Attr*> Attrs = ArrayRef<const Attr*>());
> void EmitReturnStmt(const ReturnStmt &S);
> void EmitDeclStmt(const DeclStmt &S);
> void EmitBreakStmt(const BreakStmt &S);
> @@ -1872,7 +1879,8 @@
>
> void EmitCXXTryStmt(const CXXTryStmt &S);
> void EmitSEHTryStmt(const SEHTryStmt &S);
> - void EmitCXXForRangeStmt(const CXXForRangeStmt &S);
> + void EmitCXXForRangeStmt(const CXXForRangeStmt &S,
> + ArrayRef<const Attr*> Attrs = ArrayRef<const Attr*>());
>
> llvm::Function *EmitCapturedStmt(const CapturedStmt &S, CapturedRegionKind K);
> llvm::Function *GenerateCapturedStmtFunction(const CapturedDecl *CD,
> Index: lib/Parse/ParsePragma.cpp
> ===================================================================
> --- lib/Parse/ParsePragma.cpp (revision 208124)
> +++ lib/Parse/ParsePragma.cpp (working copy)
> @@ -12,6 +12,7 @@
> //===----------------------------------------------------------------------===//
>
> #include "RAIIObjectsForParser.h"
> +#include "clang/Basic/LoopHint.h"
> #include "clang/Lex/Preprocessor.h"
> #include "clang/Parse/ParseDiagnostic.h"
> #include "clang/Parse/Parser.h"
> @@ -131,6 +132,12 @@
> Token &FirstToken) override;
> };
>
> +struct PragmaLoopHintHandler : public PragmaHandler {
> + PragmaLoopHintHandler() : PragmaHandler("loop") {}
> + void HandlePragma(Preprocessor &PP, PragmaIntroducerKind Introducer,
> + Token &FirstToken) override;
> +};
> +
> } // end namespace
>
> void Parser::initializePragmaHandlers() {
> @@ -195,6 +202,9 @@
> MSSection.reset(new PragmaMSPragma("section"));
> PP.AddPragmaHandler(MSSection.get());
> }
> +
> + LoopHintHandler.reset(new PragmaLoopHintHandler());
> + PP.AddPragmaHandler(LoopHintHandler.get());
> }
>
> void Parser::resetPragmaHandlers() {
> @@ -249,6 +259,10 @@
>
> PP.RemovePragmaHandler("STDC", FPContractHandler.get());
> FPContractHandler.reset();
> +
> + PP.RemovePragmaHandler(LoopHintHandler.get());
> + LoopHintHandler.reset();
> +
> }
>
> /// \brief Handle the annotation token produced for #pragma unused(...)
> @@ -570,6 +584,36 @@
> DiagnosticsEngine::Error, "'#pragma %0' not implemented.");
> }
>
> +struct PragmaLoopHintInfo {
> + Token Value;
> + Token Option;
> +};
> +
> +LoopHint Parser::HandlePragmaLoopHint() {
> + assert(Tok.is(tok::annot_pragma_loop_hint));
> + PragmaLoopHintInfo *Info =
> + static_cast<PragmaLoopHintInfo *>(Tok.getAnnotationValue());
> +
> + LoopHint Hint;
> + Hint.OptionLoc = IdentifierLoc::create(Actions.Context,
> + Info->Option.getLocation(),
> + Info->Option.getIdentifierInfo());
> + Hint.ValueLoc = IdentifierLoc::create(Actions.Context,
> + Info->Value.getLocation(),
> + Info->Value.getIdentifierInfo());
> + Hint.Range = SourceRange(Info->Option.getLocation(),
> + Info->Value.getLocation());
> +
> + // FIXME: We should support template parameters for the loop hint value.
> + // See bug report #19610
> + if (Info->Value.is(tok::numeric_constant))
> + Hint.ValueExpr = Actions.ActOnNumericConstant(Info->Value);
> + else
> + Hint.ValueExpr = nullptr;
> +
> + return Hint;
> +}
> +
> // #pragma GCC visibility comes in two variants:
> // 'push' '(' [visibility] ')'
> // 'pop'
> @@ -1531,3 +1575,100 @@
>
> Actions.ActOnPragmaMSComment(Kind, ArgumentString);
> }
> +
> +/// \brief Handle the \#pragma vectorize hint
> +///
> +/// The syntax is:
> +/// \code
> +/// #pragma loop vectorize(enable/disable/_value_)
> +/// #pragma loop interleave(enable/disable/_value_)
> +/// #pragma loop vectorize(...) interleave(...) ...
> +/// \endcode
> +/// Specifying vectorize(enable) or vectorize(_value_) instructs llvm to
> +/// try vectorizing the subsequent loop. Specifying interleave(enable) or
> +/// interleave(_value_) instructs llvm to tru interleaving the subsequent loop
> +/// Giving a _value_ of 1 or "disable" prevents the optimization, even if it
> +/// is possible or profitable. The loop vectorizer currently only works on
> +/// inner loops.
> +void PragmaLoopHintHandler::HandlePragma(Preprocessor &PP,
> + PragmaIntroducerKind Introducer,
> + Token &Tok) {
> + SourceLocation LoopHintLoc = Tok.getLocation();
> +
> + SmallVector <Token, 1> TokenList;
> +
> + // Lex the optimization option and verify it is an identifier.
> + PP.Lex(Tok);
> + if (Tok.isNot(tok::identifier)) {
> + PP.Diag(Tok.getLocation(), diag::warn_pragma_loop_missing_option);
> + return;
> + }
> +
> + while(Tok.is(tok::identifier)) {
> + // Read the optimization option identifier.
> + PragmaLoopHintInfo *Info =
> + (PragmaLoopHintInfo*) PP.getPreprocessorAllocator().Allocate(
> + sizeof(PragmaLoopHintInfo), llvm::alignOf<PragmaLoopHintInfo>());
> +
> + Info->Option = Tok;
> + StringRef Option = Tok.getIdentifierInfo()->getName();
> +
> + if (Option != "vectorize" && Option != "interleave") {
> + PP.Diag(Tok.getLocation(), diag::warn_pragma_loop_invalid_option) <<
> + Option;
> + return;
> + }
> +
> + // Read '('
> + PP.Lex(Tok);
> + if (Tok.isNot(tok::l_paren)) {
> + PP.Diag(Tok.getLocation(), diag::err_expected) << tok::l_paren;
> + return;
> + }
> +
> + // Read the optimization value identifier.
> + PP.Lex(Tok);
> + if (Tok.isNot(tok::identifier) && Tok.isNot(tok::numeric_constant)) {
> + PP.Diag(Tok.getLocation(), diag::warn_pragma_loop_invalid_type) <<
> + Tok.getName();
> + return;
> + }
> +
> + Info->Value = Tok;
> +
> + // Read ')'
> + PP.Lex(Tok);
> + if (Tok.isNot(tok::r_paren)) {
> + PP.Diag(Tok.getLocation(), diag::err_expected) << tok::r_paren;
> + return;
> + }
> +
> + // Get next optimization option.
> + PP.Lex(Tok);
> +
> + // Generate the vectorization hint token.
> + Token LoopHintTok;
> + LoopHintTok.startToken();
> + LoopHintTok.setKind(tok::annot_pragma_loop_hint);
> + LoopHintTok.setLocation(LoopHintLoc);
> + LoopHintTok.setAnnotationValue(static_cast<void*>(Info));
> + TokenList.push_back(LoopHintTok);
> + }
> +
> + // Consume all remaining token in the pragam loop directive.
> + if (Tok.isNot(tok::eod)) {
> + PP.Diag(Tok.getLocation(), diag::warn_pragma_extra_tokens_at_eol) <<
> + "loop";
> + return;
> + }
> +
> + Token *TokenArray = new Token[TokenList.size()];
> + for(unsigned long i = 0; i < TokenList.size(); i++) {
> + TokenArray[i] = TokenList[i];
> + }
> +
> + PP.EnterTokenStream(TokenArray, TokenList.size(),
> + /*DisableMacroExpansion=*/false,
> + /*OwnsTokens=*/true);
> + TokenList.clear();
> +}
> Index: lib/Parse/ParseStmt.cpp
> ===================================================================
> --- lib/Parse/ParseStmt.cpp (revision 208124)
> +++ lib/Parse/ParseStmt.cpp (working copy)
> @@ -15,7 +15,9 @@
> #include "clang/Parse/Parser.h"
> #include "RAIIObjectsForParser.h"
> #include "clang/AST/ASTContext.h"
> +#include "clang/Basic/Attributes.h"
> #include "clang/Basic/Diagnostic.h"
> +#include "clang/Basic/LoopHint.h"
> #include "clang/Basic/PrettyStackTrace.h"
> #include "clang/Basic/SourceManager.h"
> #include "clang/Basic/TargetInfo.h"
> @@ -357,6 +359,10 @@
> ProhibitAttributes(Attrs);
> HandlePragmaMSPragma();
> return StmtEmpty();
> +
> + case tok::annot_pragma_loop_hint:
> + ProhibitAttributes(Attrs);
> + return ParsePragmaLoopHint(Stmts, OnlyStatement, TrailingElseLoc, Attrs);
> }
>
> // If we reached this code, the statement must end in a semicolon.
> @@ -1753,6 +1759,51 @@
> return Actions.ActOnReturnStmt(ReturnLoc, R.take(), getCurScope());
> }
>
> +StmtResult Parser::ParsePragmaLoopHint(StmtVector &Stmts, bool OnlyStatement,
> + SourceLocation *TrailingElseLoc,
> + ParsedAttributesWithRange &Attrs)
> +{
Formatting.
> + // Create temporary attribute list.
> + ParsedAttributesWithRange TempAttrs(AttrFactory);
> +
> + // Get vectorize hints and consume annotated token.
> + while (Tok.is(tok::annot_pragma_loop_hint)) {
> + LoopHint Hint = HandlePragmaLoopHint();
> + ConsumeToken();
> +
> + if (!Hint.ValueLoc || !Hint.OptionLoc)
> + continue;
> +
> + ArgsUnion ArgHints[] = {Hint.ValueLoc,
> + ArgsUnion(Hint.ValueExpr.get())};
> + TempAttrs.addNew(Hint.OptionLoc->Ident, Hint.Range,
> + 0, Hint.OptionLoc->Loc,
> + ArgHints, 2,
> + AttributeList::AS_Keyword);
> + }
> +
> + // Get the next statement.
> + MaybeParseCXX11Attributes(Attrs);
> +
> + if (isTokenSpecial()) {
> + Diag(Tok.getLocation(), diag::warn_pragma_loop_precedes_nonloop);
> + return StmtEmpty();
> + }
> +
> + Token StmtTok = Tok;
> + StmtResult S = ParseStatementOrDeclarationAfterAttributes(Stmts,
> + /*OnlyStatement*/ true, 0, Attrs);
> +
> + // If it is a loop add the loop hint attributes to it.
> + if (StmtTok.is(tok::kw_for) || StmtTok.is(tok::kw_while) || StmtTok.is(tok::kw_do)) {
Formatting.
> + Attrs.takeAllFrom(TempAttrs);
> + return S;
> + }
> +
> + Diag(StmtTok.getLocation(), diag::warn_pragma_loop_precedes_nonloop);
> + return S;
> +}
> +
> namespace {
> class ClangAsmParserCallback : public llvm::MCAsmParserSemaCallback {
> Parser &TheParser;
> Index: lib/Sema/SemaStmtAttr.cpp
> ===================================================================
> --- lib/Sema/SemaStmtAttr.cpp (revision 208124)
> +++ lib/Sema/SemaStmtAttr.cpp (working copy)
> @@ -14,6 +14,9 @@
> #include "clang/Sema/SemaInternal.h"
> #include "clang/AST/ASTContext.h"
> #include "clang/Basic/SourceManager.h"
> +#include "clang/Basic/LoopHint.h"
> +#include "clang/Lex/Lexer.h"
> +#include "clang/Parse/ParseDiagnostic.h"
Why are parser diagnostics required for sema? That seems wrong.
> #include "clang/Sema/DelayedDiagnostic.h"
> #include "clang/Sema/Lookup.h"
> #include "clang/Sema/ScopeInfo.h"
> @@ -42,7 +45,42 @@
> A.getAttributeSpellingListIndex());
> }
>
> +static Attr *handleLoopHintAttr(Sema &S, Stmt *St, const AttributeList &A,
> + SourceRange Range) {
> + if (St->getStmtClass() != Stmt::DoStmtClass &&
> + St->getStmtClass() != Stmt::ForStmtClass &&
> + St->getStmtClass() != Stmt::CXXForRangeStmtClass &&
> + St->getStmtClass() != Stmt::WhileStmtClass) {
> + S.Diag(St->getLocStart(), diag::warn_pragma_loop_precedes_nonloop);
Wasn't this already warned on during parsing?
> + return 0;
> + }
>
> + IdentifierLoc *ValueLoc = A.getArgAsIdent(0);
> + IdentifierInfo *ValueInfo = ValueLoc->Ident;
> +
> + LoopHintAttr::Type Type = LoopHintAttr::Value;
> + if (ValueInfo && ValueInfo->getName() == "enable")
> + Type = LoopHintAttr::Enable;
> + else if (ValueInfo && ValueInfo->getName() == "disable")
> + Type = LoopHintAttr::Disable;
> +
> + StringRef Option = A.getName()->getName();
> + if (Option == "vectorize") {
> + return LoopHintAttr::CreateImplicit(S.Context,
> + LoopHintAttr::Keyword_vectorize,
> + Type, A.getArgAsExpr(1), A.getRange());
> + } else if (Option == "interleave") {
> + return LoopHintAttr::CreateImplicit(S.Context,
> + LoopHintAttr::Keyword_interleave,
> + Type, A.getArgAsExpr(1), A.getRange());
> + }
> +
> + S.Diag(A.getLoc(), diag::warn_pragma_loop_invalid_option) <<
> + Option;
Wasn't this already warned on during parsing as well?
> +
> + return 0;
> +}
> +
> static Attr *ProcessStmtAttribute(Sema &S, Stmt *St, const AttributeList &A,
> SourceRange Range) {
> switch (A.getKind()) {
> @@ -53,6 +91,8 @@
> return 0;
> case AttributeList::AT_FallThrough:
> return handleFallThroughAttr(S, St, A, Range);
> + case AttributeList::AT_LoopHint:
> + return handleLoopHintAttr(S, St, A, Range);
> default:
> // if we're here, then we parsed a known attribute, but didn't recognize
> // it as a statement attribute => it is declaration attribute
> Index: test/CodeGen/pragma-loop.cpp
> ===================================================================
> --- test/CodeGen/pragma-loop.cpp (revision 0)
> +++ test/CodeGen/pragma-loop.cpp (working copy)
> @@ -0,0 +1,120 @@
> +// RUN: %clang -std=c++11 -emit-llvm -S -o - %s | FileCheck %s
> +
> +// CHECK: br i1 %cmp, label %while.body, label %while.end, !llvm.loop !1
> +// CHECK: br i1 %cmp, label %do.body, label %do.end, !llvm.loop !5
> +// CHECK: br i1 %cmp, label %for.body, label %for.end, !llvm.loop !7
> +// CHECK: br i1 %cmp, label %for.body, label %for.end, !llvm.loop !8
> +// CHECK: br i1 %cmp, label %for.body, label %for.end, !llvm.loop !11
> +// CHECK: br i1 %cmp, label %for.body, label %for.end, !llvm.loop !13
> +// CHECK: br i1 %cmp, label %for.body, label %for.end, !llvm.loop !14
> +// CHECK: br i1 %cmp, label %for.body, label %for.end, !llvm.loop !16
> +
> +// CHECK: !1 = metadata !{metadata !1, metadata !2, metadata !3, metadata !4}
> +// CHECK: !2 = metadata !{metadata !"llvm.vectorizer.enable", i1 true}
> +// CHECK: !3 = metadata !{metadata !"llvm.vectorizer.unroll", i32 4}
> +// CHECK: !4 = metadata !{metadata !"llvm.vectorizer.width", i32 4}
> +// CHECK: !5 = metadata !{metadata !5, metadata !6, metadata !3}
> +// CHECK: !6 = metadata !{metadata !"llvm.vectorizer.width", i32 8}
> +// CHECK: !7 = metadata !{metadata !7, metadata !2, metadata !3}
> +// CHECK: !8 = metadata !{metadata !8, metadata !9, metadata !10}
> +// CHECK: !9 = metadata !{metadata !"llvm.vectorizer.width", i32 2}
> +// CHECK: !10 = metadata !{metadata !"llvm.vectorizer.unroll", i32 2}
> +// CHECK: !11 = metadata !{metadata !11, metadata !12}
> +// CHECK: !12 = metadata !{metadata !"llvm.vectorizer.width", i32 1}
> +// CHECK: !13 = metadata !{metadata !13, metadata !9, metadata !10}
> +// CHECK: !14 = metadata !{metadata !14, metadata !6, metadata !15}
> +// CHECK: !15 = metadata !{metadata !"llvm.vectorizer.unroll", i32 8}
> +// CHECK: !16 = metadata !{metadata !16, metadata !9, metadata !10}
> +
> +// Verify while loop is recognized after sequence of pragma loop directives.
> +void while_test(int *List, int Length) {
> + int i = 0;
> +
> + #pragma loop vectorize(enable)
> + #pragma loop interleave(4)
> + #pragma loop vectorize(4)
> + while(i < Length) {
> + List[i] = i*2;
> + i++;
> + }
> +}
> +
> +// Verify do loop is recognized after multi-option pragma loop directive.
> +void do_test(int *List, int Length) {
> + int i = 0;
> +
> + #pragma loop vectorize(8) interleave(4)
> + do {
> + List[i] = i*2;
> + i++;
> + } while (i < Length);
> +}
> +
> +// Verify for loop is recognized after sequence of pragma loop directives.
> +void for_test(int *List, int Length) {
> + #pragma loop interleave(enable)
> + #pragma loop interleave(4)
> + for(int i = 0; i < Length; i++) {
> + List[i] = i*2;
> + }
> +}
> +
> +// Verify c++11 for range loop is recognized after
> +// sequence of pragma loop directives.
> +void for_range_test() {
> + double List[100];
> +
> + #pragma loop vectorize(2) interleave(2)
> + for (int i : List) {
> + List[i] = i;
> + }
> +}
> +
> +// Verify disable pragma loop directive generates correct metadata
> +void disable_test(int *List, int Length) {
> + #pragma loop vectorize(disable)
> + for(int i = 0; i < Length; i++) {
> + List[i] = i*2;
> + }
> +}
> +
> +#define VECWIDTH 2
> +#define INTERLEAVE 2
> +
> +// Verify defines are correctly resolved in pragma loop directive
> +void for_define_test(int *List, int Length, int Value) {
> + #pragma loop vectorize(VECWIDTH) interleave(INTERLEAVE)
> + for(int i = 0; i < Length; i++) {
> + List[i] = i*Value;
> + }
> +}
> +
> +// Verify metadata is generated when template is used.
> +template <typename A>
> +void for_template_test(A *List, int Length, A Value) {
> +
> + #pragma loop vectorize(8) interleave(8)
> + for(int i = 0; i < Length; i++) {
> + List[i] = i*Value;
> + }
> +}
> +
> +// Verify define is resolved correctly when template is used.
> +template <typename A>
> +void for_template_define_test(A *List, int Length, A Value) {
> + #pragma loop vectorize(VECWIDTH) interleave(INTERLEAVE)
> + for(int i = 0; i < Length; i++) {
> + List[i] = i*Value;
> + }
> +}
> +
> +#undef VECWIDTH
> +#undef INTERLEAVE
> +
> +// Use templates defined above. Test verifies metadata is generated correctly.
> +void template_test(double *List, int Length) {
> + double Value = 10;
> +
> + for_template_test<double>(List, Length, Value);
> + for_template_define_test<double>(List, Length, Value);
> +}
> Index: test/PCH/pragma-loop.cpp
> ===================================================================
> --- test/PCH/pragma-loop.cpp (revision 0)
> +++ test/PCH/pragma-loop.cpp (working copy)
> @@ -0,0 +1,59 @@
> +// RUN: %clang_cc1 -emit-pch -o %t.a %s
> +// RUN: %clang_cc1 -include-pch %t.a %s -ast-print -o - | FileCheck %s
> +
> +// CHECK: #pragma loop vectorize(8)
> +// CHECK: #pragma loop interleave(4)
> +// CHECK: #pragma loop vectorize(disable)
> +// CHECK: #pragma loop interleave(enable)
> +// CHECK: #pragma loop vectorize(enable)
> +// CHECK: #pragma loop interleave(disable)
> +
> +#ifndef HEADER
> +#define HEADER
> +
> +class pragma_test {
> +public:
> + inline void run1(int *List, int Length) {
> + int i = 0;
> + #pragma loop vectorize(8)
> + #pragma loop interleave(4)
> + while (i < Length) {
> + List[i] = i;
> + i++;
> + }
> + }
> +
> + inline void run2(int *List, int Length) {
> + int i = 0;
> + #pragma loop vectorize(disable)
> + #pragma loop interleave(enable)
> + while (i-1 < Length) {
> + List[i] = i;
> + i++;
> + }
> + }
> +
> + inline void run3(int *List, int Length) {
> + int i = 0;
> + #pragma loop vectorize(enable)
> + #pragma loop interleave(disable)
> + while (i-3 < Length) {
> + List[i] = i;
> + i++;
> + }
> + }
> +};
> +
> +#else
> +
> +void test() {
> + int List[100];
> +
> + pragma_test pt;
> +
> + pt.run1(List, 100);
> + pt.run2(List, 100);
> + pt.run3(List, 100);
> +}
> +
> +#endif
> Index: test/Parser/pragma-loop-ast.cpp
> ===================================================================
> --- test/Parser/pragma-loop-ast.cpp (revision 0)
> +++ test/Parser/pragma-loop-ast.cpp (working copy)
> @@ -0,0 +1,15 @@
> +// RUN: %clang_cc1 -ast-print %s | FileCheck %s
> +
> +// CHECK: #pragma loop vectorize(4)
> +// CHECK: #pragma loop interleave(8)
> +
> +void test(int *List, int Length) {
> + int i = 0;
> + #pragma loop vectorize(4)
> + #pragma loop interleave(8)
> + while (i < Length)
> + {
> + List[i] = i * 2;
> + i++;
> + }
Formatting.
> +}
> Index: test/Parser/pragma-loop.cpp
> ===================================================================
> --- test/Parser/pragma-loop.cpp (revision 0)
> +++ test/Parser/pragma-loop.cpp (working copy)
> @@ -0,0 +1,61 @@
> +// RUN: %clang_cc1 -fsyntax-only -std=c++11 -verify %s
> +
> +// Note that this puts the expected lines before the directives to work around
> +// limitations in the -verify mode.
> +
> +void test(int *List, int Length)
> +{
> + int i = 0;
> +
> + #pragma loop vectorize(4)
> + #pragma loop vectorize(enable)
> + #pragma loop vectorize(disable)
> + #pragma loop interleave(8)
> + #pragma loop interleave(enable)
> + #pragma loop interleave(disable)
> + while (i-1 < Length) {
> + List[i] = i;
> + }
> +
> + #pragma loop vectorize(4) interleave(8)
> + while (i-2 < Length) {
> + List[i] = i;
> + }
> +
> + #pragma loop interleave(16)
> + while (i-3 < Length) {
> + List[i] = i;
> + }
> +
> + int VList[Length];
> + #pragma loop vectorize(disable) interleave(disable)
> + for (int j : VList) {
> + VList[j] = List[j];
> + }
> +
> +/* expected-error {{expected '('}} */ #pragma loop vectorize
> +/* expected-error {{expected '('}} */ #pragma loop interleave
> +
> +/* expected-error {{expected ')'}} */ #pragma loop vectorize(4
> +/* expected-error {{expected ')'}} */ #pragma loop interleave(4
> +
> +/* expected-warning {{missing option in directive '#pragma loop'}} */ #pragma loop
> +/* expected-warning {{invalid option 'badkeyword' in directive '#pragma loop'}} */ #pragma loop badkeyword
> +/* expected-warning {{invalid option 'badkeyword' in directive '#pragma loop'}} */ #pragma loop badkeyword(2)
> +/* expected-warning {{invalid option 'badkeyword' in directive '#pragma loop'}} */ #pragma loop vectorize(4) badkeyword(4)
> +/* expected-warning {{extra tokens at end of '#pragma loop'}} */ #pragma loop vectorize(4) ,
> +
> + while (i-4 < Length) {
> + List[i] = i;
> + }
> +
> +#pragma loop vectorize(enable)
> +/* expected-warning {{expected a for, while, or do-while loop to follow the '#pragma loop' directive}} */ int j = Length;
> + List[0] = List[1];
> +
> + while (j-5 < Length) {
> + List[j] = j;
> + }
> +
> +#pragma loop interleave(enable)
> +/* expected-warning {{expected a for, while, or do-while loop to follow the '#pragma loop' directive}} */ }
> Index: test/Parser/pragma-loop2.cpp
> ===================================================================
> --- test/Parser/pragma-loop2.cpp (revision 0)
> +++ test/Parser/pragma-loop2.cpp (working copy)
> @@ -0,0 +1,50 @@
> +// RUN: not %clang %s 2>&1 | FileCheck %s
> +
> +// Tests contraditory pragmas that are caught by the IR code generation.
> +
> +// CHECK: 'value' and 'disable' directive option types are incompatible in '#pragma loop vectorize'
> +// CHECK: 'enable' and 'disable' directive option types are incompatible in '#pragma loop interleave'
> +// CHECK: 'disable' and 'value' directive option types are incompatible in '#pragma loop vectorize'
> +// CHECK: expected a positive integer in directive '#pragma loop vectorize'
> +// CHECK: expected a positive integer in directive '#pragma loop interleave'
> +// CHECK: expected a positive integer in directive '#pragma loop vectorize'
> +// CHECK: expected a positive integer in directive '#pragma loop interleave'
> +// CHECK: expected a positive integer in directive '#pragma loop vectorize'
> +// CHECK: expected a positive integer in directive '#pragma loop interleave'
> +
> +void test(int *List, int Length)
> +{
> + int i = 0;
> +
> + #pragma loop vectorize(4)
> + #pragma loop vectorize(disable)
> + #pragma loop interleave(enable)
> + #pragma loop interleave(disable)
> + while (i-1 < Length) {
> + List[i] = i;
> + }
> +
> + #pragma loop vectorize(disable)
> + #pragma loop vectorize(4)
> + while (i-2 < Length) {
> + List[i] = i;
> + }
> +
> + #pragma loop vectorize(0)
> + #pragma loop interleave(0)
> + while (i-3 < Length) {
> + List[i] = i;
> + }
> +
> + #pragma loop vectorize(3000000000)
> + #pragma loop interleave(3000000000)
> + while (i-4 < Length) {
> + List[i] = i;
> + }
> +
> + #pragma loop vectorize(badvalue)
> + #pragma loop interleave(badvalue)
> + while (i-5 < Length) {
> + List[i] = i;
> + }
> +}
>
~Aaron
On Tue, May 6, 2014 at 8:36 PM, Ben Langmuir <blangmuir at apple.com> wrote:
> Hi Tyler,
>
> Richard already gave a pretty thorough review, but I have a few other
> questions:
>
> I think it makes sense because c++11 attributes, for example [[loop
> vectorize(4)]], would be verified at this point too.
>
>
> Why is that? Shouldn’t they both be dealt with in Sema? I don’t like the
> idea that I wouldn’t get these semantic diagnostics with -fsyntax-only.
>
> Index: test/Parser/pragma-loop2.cpp
> ===================================================================
> --- test/Parser/pragma-loop2.cpp (revision 0)
> +++ test/Parser/pragma-loop2.cpp (working copy)
> @@ -0,0 +1,50 @@
> +// RUN: not %clang %s 2>&1 | FileCheck %s
>
>
> This test just highlights why this should be done in Sema if at all
> possible… Also, you probably don’t need the driver here.
>
> + // Add vectorize hints to the metadata on the conditional branch.
> + // Iterate in reverse so hints are put in the same order they appear.
> + SmallVector<llvm::Value*, 2> Metadata(1);
> + ArrayRef<const Attr*>::reverse_iterator I;
> + for (I = Attrs.rbegin(); I != Attrs.rend(); ++I) {
> + const LoopHintAttr *LH = cast<LoopHintAttr>(*I);
>
>
> Shouldn’t this dyn_cast and continue on a nullptr result? What happens when
> I add another attribute?
>
> +def warn_pragma_loop_invalid_option : Warning<
> + "invalid option '%0' in directive '#pragma loop', expected either
> vectorize or interleave - ignoring">,
> + InGroup<PragmaLoop>;
>
>
> In this and the other diagnostics I think we want a semicolon before
> “expected” rather than a comma. Why are all the diagnostics warnings
> instead of errors?
>
> Ben
>
>
> On May 6, 2014, at 4:36 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>
> This still needs an explicit LGTM from a committer, per the developer
> policy.
>
> On Tue, May 6, 2014 at 4:23 PM, Nadav Rotem <nrotem at apple.com> wrote:
>>
>> I think that Tyler addressed all of the comments that the reviewers had,
>> so unless there are any other objections I think this patch should be
>> committed.
>>
>> On May 6, 2014, at 4:22 PM, Arnold Schwaighofer <aschwaighofer at apple.com>
>> wrote:
>>
>> It seems to me that the patch is in pretty good shape. Does it make sense
>> do the rest of the review in tree?
>>
>>
>> On May 6, 2014, at 1:19 PM, Tyler Nowicki <tnowicki at apple.com> wrote:
>>
>> Hi Alexey,
>>
>> Thanks for the review. I applied many of your suggestions. Please review
>> my comments below. Here is the updated patch.
>>
>> Tyler
>>
>> <pragma_loop-svn.patch>
>>
>>
>> + if (Type == LoopHintAttr::Value) {
>> + llvm::APSInt ValueAPS;
>> + if(!ValueExpr || !ValueExpr->EvaluateAsInt(ValueAPS,
>> CGM.getContext()) ||
>> + (ValueInt = ValueAPS.getSExtValue()) < 1) {
>> + CGM.getDiags().Report(LH->getRange().getEnd(),
>> + diag::warn_pragma_loop_invalid_value) <<
>> + LH->getSpelling();
>> + continue;
>> + }
>> + }
>> +
>> + llvm::Value *Value;
>> + llvm::MDString *Name;
>> + LoopHintAttr::Spelling S = LH->getSemanticSpelling();
>> +
>> + if (S == LoopHintAttr::Keyword_vectorize) {
>> + // Do not add hint if it is incompatible with prior hints.
>> + if (!LoopHintAttr::isCompatible(VectorizeState | Type)) {
>> + CGM.getDiags().Report(LH->getRange().getEnd(),
>> + diag::warn_pragma_loop_incompatible) <<
>> + LoopHintAttr::getName(VectorizeState) <<
>> + LoopHintAttr::getName(Type) <<
>> + LH->getSpelling();
>> + continue;
>> + }
>>
>> + } else if (S == LoopHintAttr::Keyword_interleave) {
>> + // Do not add hint if it is incompatible with prior hints.
>> + if (!LoopHintAttr::isCompatible(InterleaveState | Type)) {
>> + CGM.getDiags().Report(LH->getRange().getEnd(),
>> + diag::warn_pragma_loop_incompatible) <<
>> + LoopHintAttr::getName(InterleaveState) <<
>> + LoopHintAttr::getName(Type) <<
>> + LH->getSpelling();
>> + continue;
>> + }
>>
>>
>> I think it should be verified in Sema, not in CodeGen
>>
>>
>> I think it makes sense because c++11 attributes, for example [[loop
>> vectorize(4)]], would be verified at this point too.
>>
>>
>> 4. lib/Parse/ParsePragma.cpp
>>
>> BalancedDelimiterTracker is not used for parsing.
>>
>>
>> I looked at using this. BDT requires an instance of Parser which is not
>> given to the pragma handlers (see initializePragmaHandlers). No other
>> pragmas use BDT. If you think pragmas should use BDT then it should be done
>> in another patch.
>>
>>
>> + case tok::annot_pragma_loop_hint:
>> + ProhibitAttributes(Attrs);
>> + return ParsePragmaLoopHint(Stmts, OnlyStatement, TrailingElseLoc,
>> Attrs);
>>
>> Why attributes are prohibited?
>>
>>
>> ProhibitAttributes informs the programmer attributes are not allowed if
>> any are given. I don’t think attributes are allowed on preprocessor
>> directives ([[attribute]] #pragma …?) and none of the existing pragmas allow
>> attributes.
>>
>>
>> + if (Tok.is(tok::kw___attribute) &&
>> + (NextTok.is(tok::kw_do) ||
>> + NextTok.is(tok::kw_for) ||
>> + NextTok.is(tok::kw_while)) ) {
>> + // Get the loop's attributes.
>> + MaybeParseCXX11Attributes(Attrs, 0, /*MightBeObjCMessageSend*/ true);
>> + }
>>
>> I don't think that this correct to handle attributed statements. C++11
>> does not use __attribute as a key word for attributes, but '[[' sequence. I
>> think it would be better just to call MaybeParseCXX11Attributes(...) without
>> any preconditions. Besides, AttributedStmt will be never created, because
>> you don't call Sema::ProcessStmtAttributes(...) after all.
>>
>>
>> You are right, I improved this part of the code. But I also think you
>> missed something important about how this works. The pragma for loop hint is
>> turned into an Attrs of an AttributedStmt, but this does not happen right
>> away. The loop hint gets added to a ParsedAttributes list. If the subsequent
>> loop also has attributes those are also added to the ParsedAttributes list.
>> ParsePragmaLoopHint returns a loop stmt and the ParsedAttributes list. Both
>> are turned into an AttributedStmt by the call to ProcessStmtAttributes by
>> ParseStatementOrDeclaration.
>>
>>
>> 6. lib/Sema/SemaStmtAttr.cpp
>>
>> + if (St->getStmtClass() != Stmt::DoStmtClass &&
>> + St->getStmtClass() != Stmt::ForStmtClass &&
>> + St->getStmtClass() != Stmt::CXXForRangeStmtClass &&
>> + St->getStmtClass() != Stmt::WhileStmtClass) {
>> + S.Diag(St->getLocStart(), diag::warn_pragma_loop_precedes_nonloop);
>> + return 0;
>> + }
>>
>> AttributedStmts are not allowed?
>>
>>
>> No, this function examines the loop hint ParsedAttribute and returns an
>> Attr. The result of ProcessStmtAttributes is an AttributedStmt.
>> _______________________________________________
>> 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