… 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
