[PATCH] #pragma vectorize
Tyler Nowicki
tnowicki at apple.com
Mon May 5 11:41:49 PDT 2014
Hello Everyone,
Thank you for all the input again. Its been great and I’m learning a lot. I’ve modified the patch to handle ast-print and serialization/deserialization. Tests have been added for both of these. Several other changes have been made to improve the patch. Please review the patch and let me know if you have any concerns.
Non-type template parameters are currently not supported and will be added later.
Thanks again,
Tyler
On May 1, 2014, at 5:00 PM, Richard Smith <richard at metafoo.co.uk> wrote:
> 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/20140505/05c20d83/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pragma_loop-svn.patch
Type: application/octet-stream
Size: 41836 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140505/05c20d83/attachment.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140505/05c20d83/attachment-0001.html>
More information about the cfe-commits
mailing list