[PATCH] #pragma vectorize
Richard Smith
richard at metafoo.co.uk
Thu May 1 17:00:14 PDT 2014
On Thu, May 1, 2014 at 4:40 PM, Tyler Nowicki <tnowicki at apple.com> 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 <dgregor at apple.com> 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 <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
>
>
>
>
> _______________________________________________
> 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/6500bf20/attachment.html>
More information about the cfe-commits
mailing list