<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, May 1, 2014 at 4:40 PM, Tyler Nowicki <span dir="ltr"><<a href="mailto:tnowicki@apple.com" target="_blank">tnowicki@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word">Hi Doug,<div><br></div><div>To evaluate the Expr in the CodeGen would you use ConstantFoldsToSimpleInteger(..) to do that and get the integer value?</div>
</div></blockquote><div><br></div><div>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><span class="HOEnZb"><font color="#888888"><div>Tyler</div></font></span><div>
<div class="h5"><div><br></div><div><br><div><div>On May 1, 2014, at 10:19 AM, Douglas Gregor <<a href="mailto:dgregor@apple.com" target="_blank">dgregor@apple.com</a>> wrote:</div><br><blockquote type="cite"><div style="word-wrap:break-word">
… I somehow managed to review an old patch. Tyler and I spoke off-line and I’m looking forward to the next revision!<div><br></div><div><span style="white-space:pre-wrap"> </span>- Doug</div><div><br><div><div>On May 1, 2014, at 8:12 AM, Douglas Gregor <<a href="mailto:dgregor@apple.com" target="_blank">dgregor@apple.com</a>> wrote:</div>
<br><blockquote type="cite"><div style="word-wrap:break-word">Hi Tyler,<div><br><div><div>On Apr 29, 2014, at 3:51 PM, Tyler Nowicki <<a href="mailto:tnowicki@apple.com" target="_blank">tnowicki@apple.com</a>> wrote:</div>
<br><blockquote type="cite"><div style="word-wrap:break-word"><div>Hi,</div><div><br></div><div>I’ve updated the patch with the FIXME. I’ve also added a separate test for the contradictory pragmas.</div><div><br></div><div>
@Alexander: Since <font face="arial, sans-serif"><span style="font-size:12.800000190734863px">BalancedDelimiterTracker does not have any </span></font><font face="arial, sans-serif">benefits</font><font face="arial, sans-serif"> and just adds unnecessary complexity I opted not to use it.</font></div>
<div><font face="arial, sans-serif"><br></font></div><div>Thanks everyone for your feedback. Please review the updated patch.</div></div></blockquote><br></div></div><div><br></div><div><div>+/// LoopVectorizeHints - This provides the interface</div>
<div>+/// for specifying and retrieving vectorization hints</div><div>+///</div><div>+class LoopVectorizeHints {</div><div>+  SmallVector<VectorizeHint, 1> CondBrVectorizeHints;</div><div> </div><div>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.</div>
</div><div><br></div><div><div>+  /// Beginning of list of vectorization hints</div><div>+  SmallVectorImpl<VectorizeHint>::const_iterator beginCondBrVecHints() const {</div><div>+    return CondBrVectorizeHints.begin();</div>
<div>+  }</div><div>+</div><div>+  /// Terminator of vectorization hints list</div><div>+  SmallVectorImpl<VectorizeHint>::const_iterator endCondBrVecHints() const {</div><div>+    return CondBrVectorizeHints.end();</div>
<div>+  }</div><div><br></div><div>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 </div><div><br></div><div><span style="white-space:pre-wrap">   </span>ArrayRef<VectorizeHint> getCondBrVecHints() const { .. }</div>
<div><br></div><div>signature instead?</div><div><br></div><div><div> /// WhileStmt - This represents a 'while' stmt.</div><div> ///</div><div>-class WhileStmt : public Stmt {</div><div>+class WhileStmt : public Stmt, public LoopVectorizeHints {</div>
<div>   enum { VAR, COND, BODY, END_EXPR };</div><div><br></div><div>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.</div>
<div><br></div><div>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.,</div>
<div><br></div><div><span style="white-space:pre-wrap"> </span>llvm::DenseMap<LoopStmt *, LoopVectorizeHints> AllLoopVectorizeHints;</div></div></div><div><br></div><div>The ASTContext *does* get destroyed, so memory will get cleaned up even when you’re using SmallVector in LoopVectorizeHints.</div>
<div><br></div><div>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.</div>
<div><br></div><div><div>+enum VectorizeHintKind {</div><div>+  VH_UNKNOWN,</div><div>+  VH_ENABLE,</div><div>+  VH_DISABLE,</div><div>+  VH_WIDTH,</div><div>+  VH_UNROLL</div><div>+};</div><div><br></div><div>Doxygen comment, please! Also, the names should follow LLVM coding style:</div>
<div><br></div><div><span style="white-space:pre-wrap"> </span><a href="http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly" target="_blank">http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly</a></div>
<div><br></div><div>i.e., VH_Unknown, VH_Enable, VH_Disable.</div><div><br></div><div><div>+/// \brief Vectorization hint specified by a pragma vectorize</div><div>+///  and used by codegen to attach metadata to the IR</div>
<div>+struct VectorizeHint {</div><div>+  VectorizeHintKind Kind;</div><div>+  uint64_t Value;</div><div>+};</div></div><div><br></div><div>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.</div>
<div><br></div><div><div>+    switch(I->Kind) {</div><div>+    case VH_ENABLE:</div><div>+      Name = llvm::MDString::get(Context, "llvm.vectorizer.enable");</div><div>+      Value = llvm::ConstantInt::get(BoolTy, true);</div>
<div>+      break;</div><div>+    case VH_DISABLE:</div><div>+      Name = llvm::MDString::get(Context, "llvm.vectorizer.enable");</div><div>+      Value = llvm::ConstantInt::get(BoolTy, false);</div><div>+      break;</div>
<div>+    case VH_WIDTH:</div><div>+      Name = llvm::MDString::get(Context, "llvm.vectorizer.width");</div><div>+      Value = llvm::ConstantInt::get(IntTy, I->Value);</div><div>+      break;</div><div>+    case VH_UNROLL:</div>
<div>+      Name = llvm::MDString::get(Context, "llvm.vectorizer.unroll");</div><div>+      Value = llvm::ConstantInt::get(IntTy, I->Value);</div><div>+      break;</div><div>+    default:</div><div>+      continue;</div>
<div>+    }</div></div><div><br></div><div>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.</div>
</div><div><br></div><div><div>+    // Verify that this is one of the whitelisted vectorize hints</div><div>+    IdentifierInfo *II = Tok.getIdentifierInfo();</div><div>+    VectorizeHintKind Kind =</div><div>+      llvm::StringSwitch<VectorizeHintKind>(II->getName())</div>
<div>+      .Case("enable",  VH_ENABLE)</div><div>+      .Case("disable", VH_DISABLE)</div><div>+      .Case("width",   VH_WIDTH)</div><div>+      .Case("unroll",  VH_UNROLL)</div><div>
+      .Default(VH_UNKNOWN);</div><div><br></div><div>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?</div>
<div><br></div><div><div>+    // Verify it is a loop</div><div>+    if (isa<WhileStmt>(Loop)) {</div><div>+      WhileStmt *While = cast<WhileStmt>(Loop);</div><div>+      While->addCondBrVectorizeHint(Hint);</div>
<div>+    } else if (isa<DoStmt>(Loop)) {</div><div>+      DoStmt *Do = cast<DoStmt>(Loop);</div><div>+      Do->addCondBrVectorizeHint(Hint);</div><div>+    } else if (isa<ForStmt>(Loop)) {</div><div>
+      ForStmt *For = cast<ForStmt>(Loop);</div><div>+      For->addCondBrVectorizeHint(Hint);</div><div>+    }</div></div><div><br></div><div>This would be so much easier with a LoopStmt abstract base class.</div>
<div><br></div><div><div>+      if (Tok.isNot(tok::l_paren)) {</div><div>+        PP.Diag(Tok.getLocation(), diag::err_expected) << tok::l_paren;</div><div>+        return;</div><div>+      }</div><div>+</div><div>+      PP.Lex(Tok);</div>
<div>+      if (Tok.isNot(tok::numeric_constant) ||</div><div>+          !PP.parseSimpleIntegerLiteral(Tok, Hint->Value) ||</div><div>+          Hint->Value <= 1) {</div><div>+        PP.Diag(Tok.getLocation(), diag::err_expected) << "positive integer";</div>
<div>+      }</div><div>+</div><div>+      if (Tok.isNot(tok::r_paren)) {</div><div>+        PP.Diag(Tok.getLocation(), diag::err_expected) << tok::r_paren;</div><div>+        return;</div><div>+      }</div></div><div>
<br></div><div>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.</div>
<div><br></div><div>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.</div><div><br></div></div><div><span style="white-space:pre-wrap"> </span>- Doug</div>
<div><br></div><div><br></div></div>_______________________________________________<br>cfe-commits mailing list<br><a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</blockquote></div><br></div></div></blockquote></div><br></div></div></div></div><br>_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br></blockquote></div><br></div></div>