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
