Support non-type template parameters in #pragma loop
Aaron Ballman
aaron at aaronballman.com
Mon May 26 09:44:49 PDT 2014
Thank you for tackling this! I have some comments below.
> diff --git a/include/clang/AST/Attr.h b/include/clang/AST/Attr.h
> index 49a13f8..d905ad1 100644
> --- a/include/clang/AST/Attr.h
> +++ b/include/clang/AST/Attr.h
> @@ -18,0 +19 @@
> +#include "clang/AST/Expr.h"
> diff --git a/include/clang/Basic/Attr.td b/include/clang/Basic/Attr.td
> index 74a21e9..4afd1ea 100644
> --- a/include/clang/Basic/Attr.td
> +++ b/include/clang/Basic/Attr.td
> @@ -1790 +1790 @@ def LoopHint : PragmaAttr {
> - DefaultIntArgument<"Value", 1>];
> + ExprArgument<"Value", 0>];
> @@ -1820 +1820 @@ def LoopHint : PragmaAttr {
> - OS << value;
> + value->printPretty(OS, 0, Policy);
> diff --git a/include/clang/Sema/Sema.h b/include/clang/Sema/Sema.h
> index f1354e5..f5a7a0e 100644
> --- a/include/clang/Sema/Sema.h
> +++ b/include/clang/Sema/Sema.h
> @@ -3430,0 +3431,6 @@ public:
> +
> + bool DiagnoseLoopHintValue(SourceLocation ValueLoc, Expr *ValueExpr,
> + StringRef OptionName);
> + ExprResult ActOnLoopHintValue(Scope *S, const Token &Tok,
> + StringRef OptionName);
> +
> diff --git a/lib/CodeGen/CGStmt.cpp b/lib/CodeGen/CGStmt.cpp
> index af54b8f..5d091fd 100644
> --- a/lib/CodeGen/CGStmt.cpp
> +++ b/lib/CodeGen/CGStmt.cpp
> @@ -542 +541,0 @@ void CodeGenFunction::EmitCondBrHints(llvm::LLVMContext &Context,
> - int ValueInt = LH->getValue();
> @@ -552,0 +552,8 @@ void CodeGenFunction::EmitCondBrHints(llvm::LLVMContext &Context,
> + int ValueInt = 1;
> + if (Kind == LoopHintAttr::Value) {
> + llvm::APSInt ValueAPS;
> + Expr *ValueExpr = LH->getValue();
> + ValueExpr->isIntegerConstantExpr(ValueAPS, CGM.getContext());
It took me a while, but I understand why you aren't checking the
return value here. However, this seems like a very reasonable thing to
be asserted, just the same. A good approach is:
bool IsICE = ValueExpr->isIntegerConstantExpr(ValueAPS, CGM.getContext());
(void)IsICE; // So release build do not warn about unused variables
assert(IsICE && "Expession is not an integer constant expression");
> + ValueInt = ValueAPS.getSExtValue();
> + }
> +
> diff --git a/lib/Parse/ParsePragma.cpp b/lib/Parse/ParsePragma.cpp
> index cabba33..635edbd 100644
> --- a/lib/Parse/ParsePragma.cpp
> +++ b/lib/Parse/ParsePragma.cpp
> @@ -625,7 +625,4 @@ LoopHint Parser::HandlePragmaLoopHint() {
> -
> - // 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).get();
> - else
> - Hint.ValueExpr = nullptr;
> + Hint.ValueExpr =
> + Actions.ActOnLoopHintValue(getCurScope(), Info->Value,
> + Info->Option.getIdentifierInfo()->getName())
I think you should just pass in the IdentifierInfo * here.
> + .get();
> diff --git a/lib/Sema/SemaExpr.cpp b/lib/Sema/SemaExpr.cpp
> index 33eea16..a5ce526 100644
> --- a/lib/Sema/SemaExpr.cpp
> +++ b/lib/Sema/SemaExpr.cpp
> @@ -2974,0 +2975,59 @@ static Expr *BuildFloatingLiteral(Sema &S, NumericLiteralParser &Literal,
> +bool Sema::DiagnoseLoopHintValue(SourceLocation Loc, Expr *ValueExpr,
> + StringRef OptionName) {
This should take a const Expr * and a const IdentifierInfo *.
> + int ValueInt;
Variable is not needed.
> + llvm::APSInt ValueAPS;
> + if (!ValueExpr ||
> + !ValueExpr->isIntegerConstantExpr(ValueAPS, getASTContext()) ||
> + (ValueInt = ValueAPS.getSExtValue()) < 1) {
The assignment to ValueInt is not needed.
> + Diag(Loc, diag::err_pragma_loop_invalid_value) << OptionName;
You can pass the IdentifierInfo * in directly and the diagnostics
engine already can handle it (with correct quoting, etc).
> + return true;
> + }
> +
> + return false;
> +}
> +
> +ExprResult Sema::ActOnLoopHintValue(Scope *S, const Token &Tok,
> + StringRef OptionName) {
Should take a const IdentifierInfo * instead of a StringRef.
> + if (Tok.is(tok::numeric_constant)) {
> + ExprResult ValueExpr = ActOnNumericConstant(Tok);
> + if (DiagnoseLoopHintValue(Tok.getLocation(), ValueExpr.get(), OptionName))
> + return ExprError();
> + return ValueExpr;
> + }
> +
> + IdentifierInfo *II = Tok.getIdentifierInfo();
> + StringRef ValueName = II->getName();
> +
> + if (ValueName == "enable" || ValueName == "disable")
> + return ExprEmpty();
Can just use IdentifierInfo::isStr() instead of getting the name into
a StringRef.
> +
> + DeclarationName Name(II);
> + SourceLocation TemplateLoc = Tok.getLocation();
> +
> + LookupResult R(*this, Name, TemplateLoc, LookupOrdinaryName);
> + if (!LookupName(R, S)) {
> + Diag(Tok.getLocation(), diag::err_pragma_loop_invalid_value) << OptionName;
Is the value really invalid here? Seems more like the name is the problem.
> + return ExprError();
> + }
> +
> + NonTypeTemplateParmDecl *D = R.getAsSingle<NonTypeTemplateParmDecl>();
> +
> + if (!D || D->isParameterPack()) {
> + Diag(Tok.getLocation(), diag::err_pragma_loop_invalid_value) << OptionName;
> + return ExprError();
> + }
> +
> + CXXScopeSpec SS;
> + ExprResult DRE = BuildDeclRefExpr(D, D->getType().getNonReferenceType(),
> + VK_LValue, TemplateLoc, &SS);
> +
> + if (DRE.isInvalid()) {
> + Diag(Tok.getLocation(), diag::err_pragma_loop_invalid_value) << OptionName;
> + return ExprError();
> + }
> +
> + DRE.get()->setValueDependent(true);
Perhaps this should be using BuildDeclarationNameExpr so that we get
all of the extra checking for things like deprecation, availability,
etc? Also, should there be acessibility checks, since the lookup can
find members and those members may be visible, but inaccessible?
> +
> + return DRE;
> +}
> +
> diff --git a/lib/Sema/SemaStmtAttr.cpp b/lib/Sema/SemaStmtAttr.cpp
> index 69501fa..53868d8 100644
> --- a/lib/Sema/SemaStmtAttr.cpp
> +++ b/lib/Sema/SemaStmtAttr.cpp
> @@ -76,14 +76 @@ static Attr *handleLoopHintAttr(Sema &S, Stmt *St, const AttributeList &A,
> - // FIXME: We should support template parameters for the loop hint value.
> - // See bug report #19610
> - int ValueInt = 1; // No vectorization/interleaving when kind set to disable.
> - if (Kind == LoopHintAttr::Value) {
> - llvm::APSInt ValueAPS;
> - if (!ValueExpr || !ValueExpr->isIntegerConstantExpr(ValueAPS, S.Context) ||
> - (ValueInt = ValueAPS.getSExtValue()) < 1) {
> - S.Diag(ValueLoc->Loc, diag::err_pragma_loop_invalid_value)
> - << LoopHintAttr::getOptionName(Option);
> - return nullptr;
> - }
> - }
> -
> - return LoopHintAttr::CreateImplicit(S.Context, Option, Kind, ValueInt,
> + return LoopHintAttr::CreateImplicit(S.Context, Option, Kind, ValueExpr,
> diff --git a/lib/Sema/SemaTemplateInstantiate.cpp b/lib/Sema/SemaTemplateInstantiate.cpp
> index 657ea64..caf2531 100644
> --- a/lib/Sema/SemaTemplateInstantiate.cpp
> +++ b/lib/Sema/SemaTemplateInstantiate.cpp
> @@ -769,0 +770,4 @@ namespace {
> + /// \brief Transform the given attribute by identifying its type
> + /// and doing a custom transformation of its parameters.
> + const Attr *TransformAttr(const Attr *S);
> +
> @@ -1129,0 +1134,25 @@ TemplateInstantiator::TransformTemplateParmRefExpr(DeclRefExpr *E,
> +const Attr *TemplateInstantiator::TransformAttr(const Attr *A) {
> + if (const LoopHintAttr *LH = dyn_cast<LoopHintAttr>(A)) {
> + // Only transform non-type template parameters.
> + if (!isa<DeclRefExpr>(LH->getValue()))
> + return A;
> +
> + Expr *TransformedExpr = getDerived().TransformExpr(LH->getValue()).get();
> +
> + // Generate error if there is a problem with the value.
> + StringRef OptionName = LoopHintAttr::getOptionName(LH->getOption());
> + if (getSema().DiagnoseLoopHintValue(LH->getRange().getEnd(),
> + TransformedExpr, OptionName))
I see why DiagnoseLoopHintValue takes a StringRef now, but I would
still slightly prefer it to take an IdentifierInfo because that tracks
more information. If that turns out to be a royal pain, you can
disregard some of my previous comments on the topic.
> + return A;
> +
> + // Create new LoopHintAttr with integral expression in place of the non-type
> + // template parameter.
> + const Attr *TransformedA = LoopHintAttr::CreateImplicit(
> + getSema().Context, LH->getOption(), LH->getKind(), TransformedExpr,
> + LH->getRange());
> + return TransformedA;
Might as well just return CreateImplicit's results directly.
> + }
> +
> + return A;
> +}
> +
> diff --git a/lib/Sema/TreeTransform.h b/lib/Sema/TreeTransform.h
> index 1764272..a2ae878 100644
> --- a/lib/Sema/TreeTransform.h
> +++ b/lib/Sema/TreeTransform.h
> @@ -329,0 +330,8 @@ public:
> + /// \brief Transform the given attribute.
> + ///
> + /// By default, this routine is a place holder for transforming attributes.
> + /// Subclasses should override this function to transform specific attributes.
> + ///
> + /// \returns the transformed attribute
> + const Attr *TransformAttr(const Attr *S);
The comment doesn't match the behavior, but the behavior is correct.
You should be able to write TransformLoopHintAttr(const LoopHintAttr
*LH) and have that called (so implementers don't need to do a dyn_cast
for all kinds of attributes they may not care about). This should be
modeled after TransformStmt, where tablegen spits out the proper
AttrNodes.inc.
The Transform functionality should possibly be an independent patch,
especially once it starts to involve tablegen, etc.
> +
> @@ -5426,3 +5434,17 @@ TreeTransform<Derived>::TransformLabelStmt(LabelStmt *S) {
> -template<typename Derived>
> -StmtResult
> -TreeTransform<Derived>::TransformAttributedStmt(AttributedStmt *S) {
> +template <typename Derived>
> +const Attr *TreeTransform<Derived>::TransformAttr(const Attr *A) {
> + return A;
> +}
> +
> +template <typename Derived>
> +StmtResult TreeTransform<Derived>::TransformAttributedStmt(AttributedStmt *S) {
I think that declaration attributes should get the same treatment.
> + bool AttrsChanged = false;
> + SmallVector<const Attr *, 1> Attrs;
> +
> + // Visit attributes and keep track if any are transformed.
> + for (auto I : S->getAttrs()) {
Should be const auto *I.
> + const Attr *R = getDerived().TransformAttr(I);
> + AttrsChanged |= (I != R);
> + Attrs.push_back(R);
> + }
> +
> @@ -5433,2 +5455 @@ TreeTransform<Derived>::TransformAttributedStmt(AttributedStmt *S) {
> - // TODO: transform attributes
> - if (SubStmt.get() == S->getSubStmt() /* && attrs are the same */)
> + if (SubStmt.get() == S->getSubStmt() && !AttrsChanged)
> @@ -5437,3 +5458,2 @@ TreeTransform<Derived>::TransformAttributedStmt(AttributedStmt *S) {
> - return getDerived().RebuildAttributedStmt(S->getAttrLoc(),
> - S->getAttrs(),
> - SubStmt.get());
> + return getDerived().RebuildAttributedStmt(
> + S->getAttrLoc(), ArrayRef<const Attr *>(Attrs), SubStmt.get());
I think ArrayRef's constructor will implicitly convert from
SmallVector, so the cast isn't required.
> diff --git a/test/PCH/pragma-loop.cpp b/test/PCH/pragma-loop.cpp
> index 4f852fe..db539a9 100644
> --- a/test/PCH/pragma-loop.cpp
> +++ b/test/PCH/pragma-loop.cpp
> @@ -12,0 +13,2 @@
> +// CHECK: #pragma loop interleave(I)
> +// CHECK: #pragma loop vectorize(V)
> @@ -47,0 +50,9 @@ public:
> +
> + template <int V, int I>
> + inline void run4(int *List, int Length) {
> +#pragma loop vectorize(V)
> +#pragma loop interleave(I)
> + for (int i = 0; i < Length; i++) {
> + List[i] = i;
> + }
> + }
> @@ -59,0 +71 @@ void test() {
> + pt.run4<2, 4>(List, 100);
> diff --git a/test/Parser/pragma-loop-ast.cpp b/test/Parser/pragma-loop-ast.cpp
> index d0f293d..999c87f 100644
> --- a/test/Parser/pragma-loop-ast.cpp
> +++ b/test/Parser/pragma-loop-ast.cpp
> @@ -11,0 +12,2 @@
> +// CHECK: #pragma loop interleave(I)
> +// CHECK: #pragma loop vectorize(V)
> @@ -35,0 +38,12 @@ void test(int *List, int Length) {
> +
> +template <int V, int I>
> +void test_nontype_template_param(int *List, int Length) {
> +#pragma loop vectorize(V) interleave(I)
> + for (int i = 0; i < Length; i++) {
> + List[i] = i;
> + }
> +}
> +
> +void test_templates(int *List, int Length) {
> + test_nontype_template_param<2, 4>(List, Length);
> +}
> diff --git a/test/Parser/pragma-loop.cpp b/test/Parser/pragma-loop.cpp
> index 7c30bd3..f09fc82 100644
> --- a/test/Parser/pragma-loop.cpp
> +++ b/test/Parser/pragma-loop.cpp
> @@ -5,0 +6,24 @@
> +template <int V, int I>
> +void test_nontype_template_param(int *List, int Length) {
> +#pragma loop vectorize(V) interleave(I)
> + for (int i = 0; i < Length; i++) {
> + List[i] = i;
> + }
> +}
> +
> +template <int V, int I>
> +void test_nontype_template_vectorize(int *List, int Length) {
> + /* expected-error {{expected a positive integer in directive '#pragma loop vectorize'}} */ #pragma loop vectorize(V) interleave(I)
> + for (int i = 0; i < Length; i++) {
> + List[i] = i;
> + }
> +}
> +
> +template <int V, int I>
> +void test_nontype_template_interleave(int *List, int Length) {
> + /* expected-error {{expected a positive integer in directive '#pragma loop interleave'}} */ #pragma loop vectorize(V) interleave(I)
> + for (int i = 0; i < Length; i++) {
> + List[i] = i;
> + }
> +}
> +
> @@ -42,0 +67,2 @@ void test(int *List, int Length) {
> + test_nontype_template_param<4, 8>(List, Length);
> +
> @@ -76,0 +103,4 @@ void test(int *List, int Length) {
> +/* expected-note {{in instantiation of function template specialization}} */ test_nontype_template_vectorize<-1,8>(List, Length);
> +
> +/* expected-note {{in instantiation of function template specialization}} */ test_nontype_template_interleave<4,-1>(List, Length);
> +
>
All of the tests are using template names, but there's nothing
preventing your lookup from working on other ordinary names. Should
have some tests (both success and failure) for those. Also, I don't
see any tests where the lookups outright fail.
Thanks!
~Aaron
On Fri, May 23, 2014 at 5:41 PM, Tyler Nowicki <tnowicki at apple.com> wrote:
> Hi,
>
> As requested here is the patch to support non-type template parameters in #pragma loop. The patch uses LookupName to build a DeclRefExpr to the template parameter and overloads TransformAttr in the template instantiator to transform the value expression and replace the LoopHintAttr. It is built on the PragmaAttr and Pragma Spelling patch listed in the 'Add PragmaAttr and Pragma Spelling to Tablegen’ thread. Since it is a git patch please use ‘patch -p1’ to apply it.
>
>
>
>
> Thanks,
>
> Tyler
>
More information about the cfe-commits
mailing list