LGTM, but wait for acks from Aaron and Richard. Ben
On May 12, 2014, at 4:20 PM, Tyler Nowicki <[email protected]> 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
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
