[PATCH] #pragma vectorize
Douglas Gregor
dgregor at apple.com
Thu May 1 10:19:20 PDT 2014
… 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 <dgregor at apple.com> wrote:
> Hi Tyler,
>
> On Apr 29, 2014, at 3:51 PM, Tyler Nowicki <tnowicki at apple.com> 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
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140501/8bebec88/attachment.html>
More information about the cfe-commits
mailing list