<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div></div><div><br></div><div><br><blockquote type="cite"><div style="font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><blockquote type="cite">That part of tablegen probably shouldn’t be specialized for each type of<br>pragma should it? But from your comment below it sounds like thats what you<br>are thinking.<br></blockquote><br>I'd have to think about it more, but it seems like tablegen shouldn't<br>have to specialize for each pragma, just all pragmas. Eg) the<br>difference between printing pragmas and printing attributes is minor<br>enough that it could be handled entirely by tablegen without the<br>pragma authors having to write special code.<br></div></blockquote><div><br></div><div>I’m pretty sure that each type of pragma has a unique syntax that makes it difficult to generalize.</div><div><br></div><div><br></div><blockquote type="cite"><div style="font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><blockquote type="cite">+ ["disable", "enable", "value"],<br>+ ["Disable", "Enable", "Value"]>,<br></blockquote><br>This is actually an optional argument as well, but is not marked as<br>such. It should get a , 1. Also, this suggests we need a new argument<br>type that represents a union of arguments, since that's really what<br>you want (one of these two arguments must be used, but you don't care<br>which). A FIXME would probably be appropriate (though you don't have<br>to implement the functionality for this patch).<br></div></blockquote><div><br></div><div>I don’t think it is. Just specifying vectorize or interleave does not imply a default action. Perhaps it should?</div><div><br></div><br><blockquote type="cite"><div style="font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><blockquote type="cite">+ ExprArgument<"Value", 1>];<br></blockquote><br>Judging by the tests, this should be a DefaultIntArgument<"Value", 1>.<br>Either that, or there are tests missing where expressions are used<br>(and honestly, it would strike me as slightly strange to allow general<br>expressions here).<br></div></blockquote><div><br></div><div>I was thinking ahead to non-type template arguments. But that can wait. I’ll use an int for now.</div><div><br></div><div><br></div><blockquote type="cite"><div style="font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">const char *Names[] = { "llvm.vectorizer.width", "llvm.vectorizer.unroll" };<br>llvm::Value *Value;<br>llvm::MDString *Name;<br><br>if (Kind == LoopHintAttr::Enable) {<br> Name = llvm::MDString::get(Context, "llvm.vectorizer.enable");<br> Value = Builder.getTrue();<br>} else {<br> Name = llvm::MDString::get(Context, Names[Option]);<br> Value = llvm::ConstantInt::get(Int32Ty, ValueInt); // You already<br>set ValueInt to 1 by default, and overwrite when the Kind is a Value.<br>}<br></div></blockquote><br><div>Good idea!</div><div><br></div><div><br></div><blockquote type="cite"><div style="font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><blockquote type="cite">+ }<br>+<br>+ // Get the next statement.<br>+ MaybeParseCXX11Attributes(Attrs);<br>+<br>+ StmtResult S = ParseStatementOrDeclarationAfterAttributes(Stmts,<br>+ /*OnlyStatement*/ true, 0, Attrs);<br></blockquote><br>Shouldn't we be passing the OnlyStatement which was passed into the<br>function? Same for passing in the TrailingElseLoc instead of 0?<br></div></blockquote><div><br></div><div>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.</div><div><br></div><div><br></div><blockquote type="cite"><div style="font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">Btw, when I test your patch locally, I get failed assertions from the<br>STL. "array iterator + offset out of range" on a call to std::copy<br>within ASTStmtReader::VisitAttributedStmt().<br></div></blockquote><div><br></div><div>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.</div><div><br></div><div>Thanks,</div></div><br><div>Tyler</div></body></html>