<html><head><meta http-equiv="Content-Type" content="text/html charset=iso-8859-1"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div>Hi Mark,</div><div><br></div><div>Do you have an updated patch that would be applied on top of the constant expression patch? Perhaps you could post that so we can get a head start on reviewing it?</div><div><br></div><div>Thanks for extending the diagnostics and pragmas to unrolling!</div><div><br></div><div>Tyler</div><br><div><div>On Jul 18, 2014, at 10:54 AM, Mark Heffernan <<a href="mailto:meheff@google.com">meheff@google.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div dir="ltr">Ping? Aaron, any more comments on this?<div><br></div><div>Thanks,</div><div>Mark</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Jul 15, 2014 at 2:09 PM, Mark Heffernan <span dir="ltr"><<a href="mailto:meheff@google.com" target="_blank">meheff@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">I added a warning if the wrong pragma unroll syntax is used with CUDA. When Tyler's change adding support for expressions in loop pragmas lands, I'll add a warning for the case when the pragma argument is not an integer literal in CUDA mode. Responses to Aaron's comments inline.<br>
<div class=""><br>
On Wed, Jul 9, 2014 at 7:42 AM, Aaron Ballman <<a href="mailto:aaron.ballman@gmail.com">aaron.ballman@gmail.com</a>> wrote:<br>
</div><div class="">> On Wed, Jul 9, 2014 at 2:14 AM, Mark Heffernan <<a href="mailto:meheff@google.com">meheff@google.com</a>> wrote:<br>
</div><div class="">>> I also added "#pragma nounroll" as both icc and xlc support this pragma.<br>
><br>
> Please split nounroll out into a separate patch. The idea is certainly<br>
> reasonable, but it's preferable for patches to cover a limited,<br>
> incremental change.<br>
<br>
</div>Done.<br>
<div class=""><br>
>> On Wed, Jul 2, 2014 at 7:54 AM, Aaron Ballman <<a href="mailto:aaron.ballman@gmail.com">aaron.ballman@gmail.com</a>> wrote:<br>
> Okay, I can see why that would happen. ClangAtterEmitter.cpp generates<br>
> code for serialization and deserialization, but it doesn't have any<br>
> way of handling additional members (since those are just copy/pasted<br>
> into the attribute class definition itself).<br>
><br>
</div><div class="">> I wonder how horrible it would be to simply canonicalize based on<br>
> compiler options when pretty printing, and skip this field entirely.<br>
> Eg) when CUDA is on, pretty print does not emit the parens. When CUDA<br>
> mode is off, it does emit the parens. Yes, this isn't *exactly* what<br>
> the user wrote, but the semantics are identical either way.<br>
<br>
</div>It's now cannonicalized to the form with parentheses '#pragma unroll(n)'. Tracking of the existence of parentheses is gone.<br>
<div class=""><br>
>> Index: include/clang/Basic/DiagnosticParseKinds.td<br>
>> ===================================================================<br>
>> --- include/clang/Basic/DiagnosticParseKinds.td<br>
>> +++ include/clang/Basic/DiagnosticParseKinds.td<br>
>> @@ -811,6 +811,9 @@<br>
>> InGroup<IgnoredPragmas>;<br>
>> def warn_pragma_expected_punc : Warning<<br>
>> "expected ')' or ',' in '#pragma %0'">, InGroup<IgnoredPragmas>;<br>
>> +// - Generic errors<br>
>> +def err_pragma_missing_argument : Error<<br>
>> + "missing argument to %0 directive">;<br>
><br>
> Since it's a generic parsing diagnostic, presumably we know what kind<br>
> of argument is missing, and we could tell the user that information.<br>
> What's more, since pragmas can have multiple arguments, this doesn't<br>
> say *which* argument is missing (there could be more than one).<br>
><br>
> I also think the word "directive" could be dropped.<br>
<br>
</div>Created a generic diagnostic: "missing argument to '#pragma %0'; expected %1". This new warning is also now shared with one pragma optimize warning. Regarding stating which argument is missing, all of the pragmas using this warning only take a single argument. I think it makes sense not to specify argument position unless multiple arguments are allowed. That is, "missing first argument to '#pragma unroll'" suggests pragma unroll accepts more than one argument. If this error is later used for pragmas with multiple parameters, the format string can be adjusted accordingly.<br>
<div class=""><br>
>> def err_pragma_loop_precedes_nonloop : Error<<br>
>> - "expected a for, while, or do-while loop to follow the '#pragma clang loop' "<br>
>> - "directive">;<br>
>> + "expected a for, while, or do-while loop to follow the '%0' directive">;<br>
><br>
> I would drop "directive" (and "the") here too.<br>
<br>
</div><div class="">Done.<br>
<br>
<a href="http://reviews.llvm.org/D4297" target="_blank">http://reviews.llvm.org/D4297</a><br>
<br>
Files:<br>
docs/ReleaseNotes.rst<br>
include/clang/Basic/Attr.td<br>
include/clang/Basic/AttrDocs.td<br>
</div> include/clang/Basic/DiagnosticGroups.td<br>
<div class=""> include/clang/Basic/DiagnosticParseKinds.td<br>
include/clang/Basic/DiagnosticSemaKinds.td<br>
include/clang/Parse/Parser.h<br>
include/clang/Sema/LoopHint.h<br>
lib/Parse/ParsePragma.cpp<br>
lib/Parse/ParseStmt.cpp<br>
lib/Sema/SemaStmtAttr.cpp<br>
test/CodeGen/pragma-unroll.cpp<br>
test/PCH/pragma-loop.cpp<br>
test/Parser/pragma-loop.cpp<br>
test/Parser/pragma-unroll.cpp<br>
</div> test/Parser/<a href="http://warn-cuda-compat.cu/" target="_blank">warn-cuda-compat.cu</a><br>
</blockquote></div><br></div>
</blockquote></div><br></body></html>