[PATCH] #pragma vectorize

[Richard Smith]

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 <http://tok.is/>(tok::kw___attribute) &&
> +      (NextTok.is <http://nexttok.is/>(tok::kw_do) ||
> +       NextTok.is <http://nexttok.is/>(tok::kw_for) ||
> +       NextTok.is <http://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
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140506/66890699/attachment.html>