[PATCH] #pragma vectorize

[Ben Langmuir]

LGTM, but wait for acks from Aaron and Richard.

Ben

On May 12, 2014, at 4:20 PM, Tyler Nowicki <tnowicki at apple.com> wrote:

> Hi Aaron, Ben,
> 
> Thank again for the review. I’ve made the changes you suggested.
> 
> There was a question about why I reversed the iteration over the attribute list. The answer is that the attribute list built by ParsedAttributes stores attributes in reverse order. However, this is wrong because the correct order should be maintained for serialization/deserialization and error reporting. So we have to iterate rbegin->rend. Ideally the ParsedAttributes list should be fixed to store attributes in the order they appear but this looks like a lot of work.
> 
> Tyler
> 
> <pragma_loop-svn.patch>
> 
> 
>>> That part of tablegen probably shouldn’t be specialized for each type of
>>> pragma should it? But from your comment below it sounds like thats what you
>>> are thinking.
>> 
>> I'd have to think about it more, but it seems like tablegen shouldn't
>> have to specialize for each pragma, just all pragmas. Eg) the
>> difference between printing pragmas and printing attributes is minor
>> enough that it could be handled entirely by tablegen without the
>> pragma authors having to write special code.
> 
> I’m pretty sure that each type of pragma has a unique syntax that makes it difficult to generalize.
> 
> 
>>> +                          ["disable", "enable", "value"],
>>> +                          ["Disable", "Enable", "Value"]>,
>> 
>> This is actually an optional argument as well, but is not marked as
>> such. It should get a , 1. Also, this suggests we need a new argument
>> type that represents a union of arguments, since that's really what
>> you want (one of these two arguments must be used, but you don't care
>> which). A FIXME would probably be appropriate (though you don't have
>> to implement the functionality for this patch).
> 
> I don’t think it is. Just specifying vectorize or interleave does not imply a default action. Perhaps it should?
> 
> 
>>> +              ExprArgument<"Value", 1>];
>> 
>> Judging by the tests, this should be a DefaultIntArgument<"Value", 1>.
>> Either that, or there are tests missing where expressions are used
>> (and honestly, it would strike me as slightly strange to allow general
>> expressions here).
> 
> I was thinking ahead to non-type template arguments. But that can wait. I’ll use an int for now.
> 
> 
>> const char *Names[] = { "llvm.vectorizer.width", "llvm.vectorizer.unroll" };
>> llvm::Value *Value;
>> llvm::MDString *Name;
>> 
>> if (Kind == LoopHintAttr::Enable) {
>>  Name = llvm::MDString::get(Context, "llvm.vectorizer.enable");
>>  Value = Builder.getTrue();
>> } else {
>>  Name = llvm::MDString::get(Context, Names[Option]);
>>  Value = llvm::ConstantInt::get(Int32Ty, ValueInt); // You already
>> set ValueInt to 1 by default, and overwrite when the Kind is a Value.
>> }
> 
> Good idea!
> 
> 
>>> +  }
>>> +
>>> +  // Get the next statement.
>>> +  MaybeParseCXX11Attributes(Attrs);
>>> +
>>> +  StmtResult S = ParseStatementOrDeclarationAfterAttributes(Stmts,
>>> +                  /*OnlyStatement*/ true, 0, Attrs);
>> 
>> Shouldn't we be passing the OnlyStatement which was passed into the
>> function? Same for passing in the TrailingElseLoc instead of 0?
> 
> These inputs confused me, I duplicated the call made in ParseLabeledStatement(). I think OnlyStatement indicates that the next thing parsed is expected be a statement, rather than a declaration. I’ll pass the arguments as you suggest.
> 
> 
>> Btw, when I test your patch locally, I get failed assertions from the
>> STL. "array iterator + offset out of range" on a call to std::copy
>> within ASTStmtReader::VisitAttributedStmt().
> 
> I don’t seem to have that, also I didn’t make any changes to ASTStmtReader? Could you try out the attached patch and provide the stack dump if it fails again.
> 
> Thanks,
> 
> Tyler

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140512/0e0d23a2/attachment.html>