This still needs an explicit LGTM from a committer, per the developer policy.
On Tue, May 6, 2014 at 4:23 PM, Nadav Rotem <[email protected]> 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 <[email protected]> > 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 <[email protected]> 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 > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > > > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
