[PATCH] #pragma vectorize

[Arnold Schwaighofer]

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140506/4e88dfd7/attachment.html>