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(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 > [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
