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