On Thu, May 1, 2014 at 4:40 PM, Tyler Nowicki <[email protected]> wrote:
> Hi Doug, > > To evaluate the Expr in the CodeGen would you > use ConstantFoldsToSimpleInteger(..) to do that and get the integer value? > You should presumably be checking that the expression is an integral constant expression during template instantiation, and not waiting until CodeGen. For that, use Sema::VerifyIntegerConstantExpression. > 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 > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
