Hi Doug, To evaluate the Expr in the CodeGen would you use ConstantFoldsToSimpleInteger(..) to do that and get the integer value?
Tyler On May 1, 2014, at 10:19 AM, Douglas Gregor <[email protected]> wrote: > … I somehow managed to review an old patch. Tyler and I spoke off-line and > I’m looking forward to the next revision! > > - Doug > > On May 1, 2014, at 8:12 AM, Douglas Gregor <[email protected]> wrote: > >> Hi Tyler, >> >> On Apr 29, 2014, at 3:51 PM, Tyler Nowicki <[email protected]> wrote: >> >>> Hi, >>> >>> I’ve updated the patch with the FIXME. I’ve also added a separate test for >>> the contradictory pragmas. >>> >>> @Alexander: Since BalancedDelimiterTracker does not have any benefits and >>> just adds unnecessary complexity I opted not to use it. >>> >>> Thanks everyone for your feedback. Please review the updated patch. >> >> >> +/// LoopVectorizeHints - This provides the interface >> +/// for specifying and retrieving vectorization hints >> +/// >> +class LoopVectorizeHints { >> + SmallVector<VectorizeHint, 1> CondBrVectorizeHints; >> >> AST nodes are never destroyed, so any memory they refer to must be allocated >> through the ASTContext. SmallVectors or other memory-holding data structures >> in AST nodes will leak. Please use either an ArrayRef that refers to >> ASTContext-allocated memory or (if you must resize after initializing >> constructing the AST node) ASTVector for this. However, hold that thought… >> better idea coming below. >> >> + /// Beginning of list of vectorization hints >> + SmallVectorImpl<VectorizeHint>::const_iterator beginCondBrVecHints() >> const { >> + return CondBrVectorizeHints.begin(); >> + } >> + >> + /// Terminator of vectorization hints list >> + SmallVectorImpl<VectorizeHint>::const_iterator endCondBrVecHints() const { >> + return CondBrVectorizeHints.end(); >> + } >> >> We tend to use STL-ish “_begin” and “_end” when naming the functions that >> get iterators. Do you want to provide just the for-range-loop-friendly >> >> ArrayRef<VectorizeHint> getCondBrVecHints() const { .. } >> >> signature instead? >> >> /// WhileStmt - This represents a 'while' stmt. >> /// >> -class WhileStmt : public Stmt { >> +class WhileStmt : public Stmt, public LoopVectorizeHints { >> enum { VAR, COND, BODY, END_EXPR }; >> >> I see that WhileStmt, DoWhileStmt, and ForStmt are covered. I assume that >> CXXForRangeStmt and ObjCForCollectionStmt should also get this behavior, >> which implies that we should just bite the bullet and add an abstract class >> LoopStmt from which these all inherit and where this functionality lives. >> >> We shouldn’t bloat the size of every WhileStmt for the (extremely rare) case >> where the loop has vectorization hints. Here’s an alternative approach: add >> a bit down in Stmt (e.g., in a new LoopStmtBitFields) that indicates the >> presence of loop vectorization hints. Then, add to ASTContext a DenseMap >> from LoopStmt*’s with this bit set to the corresponding LoopVectorizeHints >> structure, i.e., >> >> llvm::DenseMap<LoopStmt *, LoopVectorizeHints> AllLoopVectorizeHints; >> >> The ASTContext *does* get destroyed, so memory will get cleaned up even when >> you’re using SmallVector in LoopVectorizeHints. >> >> The accessors to get at the LoopVectorizeHints element for a LoopStmt should >> still be on the LoopStmt node, so the API is the same, but it costs nothing >> in the common case (the bit you’ll be stealing is just padding now). This is >> how we handle declaration attributes, among other things. It’s a good >> pattern. >> >> +enum VectorizeHintKind { >> + VH_UNKNOWN, >> + VH_ENABLE, >> + VH_DISABLE, >> + VH_WIDTH, >> + VH_UNROLL >> +}; >> >> Doxygen comment, please! Also, the names should follow LLVM coding style: >> >> >> http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly >> >> i.e., VH_Unknown, VH_Enable, VH_Disable. >> >> +/// \brief Vectorization hint specified by a pragma vectorize >> +/// and used by codegen to attach metadata to the IR >> +struct VectorizeHint { >> + VectorizeHintKind Kind; >> + uint64_t Value; >> +}; >> >> Alexey is right that this will need to change to support non-type template >> arguments. You’ll likely end up with an Expr* here instead of Value, and >> will use the constant evaluator in CodeGen to get the value. I’m okay with >> this coming in a follow-up patch. >> >> + switch(I->Kind) { >> + case VH_ENABLE: >> + Name = llvm::MDString::get(Context, "llvm.vectorizer.enable"); >> + Value = llvm::ConstantInt::get(BoolTy, true); >> + break; >> + case VH_DISABLE: >> + Name = llvm::MDString::get(Context, "llvm.vectorizer.enable"); >> + Value = llvm::ConstantInt::get(BoolTy, false); >> + break; >> + case VH_WIDTH: >> + Name = llvm::MDString::get(Context, "llvm.vectorizer.width"); >> + Value = llvm::ConstantInt::get(IntTy, I->Value); >> + break; >> + case VH_UNROLL: >> + Name = llvm::MDString::get(Context, "llvm.vectorizer.unroll"); >> + Value = llvm::ConstantInt::get(IntTy, I->Value); >> + break; >> + default: >> + continue; >> + } >> >> Please replace the “default:” with “case VH_UNKNOWN:”. We like to fully >> cover our enums in switches, so that when someone adds a new VH_* constant, >> compiler warnings direct them to everything that needs to be updated. >> >> + // Verify that this is one of the whitelisted vectorize hints >> + IdentifierInfo *II = Tok.getIdentifierInfo(); >> + VectorizeHintKind Kind = >> + llvm::StringSwitch<VectorizeHintKind>(II->getName()) >> + .Case("enable", VH_ENABLE) >> + .Case("disable", VH_DISABLE) >> + .Case("width", VH_WIDTH) >> + .Case("unroll", VH_UNROLL) >> + .Default(VH_UNKNOWN); >> >> Since we’re not actually creating VH_UNKNOWNs in the AST, there’s no reason >> to have VH_UNKNOWN. Why not make this StringSwitch produce an >> Optional<VectorizeHintKind>, so VK_UNKNOWN can go away? >> >> + // Verify it is a loop >> + if (isa<WhileStmt>(Loop)) { >> + WhileStmt *While = cast<WhileStmt>(Loop); >> + While->addCondBrVectorizeHint(Hint); >> + } else if (isa<DoStmt>(Loop)) { >> + DoStmt *Do = cast<DoStmt>(Loop); >> + Do->addCondBrVectorizeHint(Hint); >> + } else if (isa<ForStmt>(Loop)) { >> + ForStmt *For = cast<ForStmt>(Loop); >> + For->addCondBrVectorizeHint(Hint); >> + } >> >> This would be so much easier with a LoopStmt abstract base class. >> >> + if (Tok.isNot(tok::l_paren)) { >> + PP.Diag(Tok.getLocation(), diag::err_expected) << tok::l_paren; >> + return; >> + } >> + >> + PP.Lex(Tok); >> + if (Tok.isNot(tok::numeric_constant) || >> + !PP.parseSimpleIntegerLiteral(Tok, Hint->Value) || >> + Hint->Value <= 1) { >> + PP.Diag(Tok.getLocation(), diag::err_expected) << "positive >> integer"; >> + } >> + >> + if (Tok.isNot(tok::r_paren)) { >> + PP.Diag(Tok.getLocation(), diag::err_expected) << tok::r_paren; >> + return; >> + } >> >> I think Alexander is right about BalancedDelimiterTracker. Among other >> things, it gives better recovery on overly-nested code and provides better >> diagnostics and recovery when the ‘)’ is missing than your hand-coded >> solution. >> >> As Alexey notes, (de-)serialization and AST printing and AST dumping are >> important to handle. I’m okay with those being follow-up patches as well. >> >> - Doug >> >> >> _______________________________________________ >> 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
