<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Jun 26, 2014 at 7:47 AM, Aaron Ballman <span dir="ltr"><<a href="mailto:aaron.ballman@gmail.com" target="_blank">aaron.ballman@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div>On Wed, Jun 25, 2014 at 4:28 PM, Mark Heffernan <<a href="mailto:meheff@google.com" target="_blank">meheff@google.com</a>> wrote:<br>
> This pragma is only supported when compiling in CUDA mode (-x cuda). The "#pragma unroll" and "#pragma unroll N" have identical semantics to "#pragma clang loop unroll(enable)" and "#pragma clang loop unroll_count(N)" respectively.<br>
<br>
</div>I am really uncomfortable with the idea of having two pragmas with<br>
identical semantics but differing syntax. I would prefer that we have<br>
a single syntax since it is *identical* functionality. I suspect this<br>
pragma is driven by the CUDA standard? If so, has the idea been<br>
explored of simply supporting CUDA's syntax outside of CUDA mode, and<br>
dropping #pramga loop unroll?<br></blockquote><div><br></div><div>CUDA defines the unroll pragma this way. For GPU code, supporting it is essential. For about half of our internal benchmarks performance is > 10x slower if the pragma is not supported, and this is probably not atypical for tuned code. Also, making the pragma syntax identical to CUDA is clearly important for compatibility with nvcc and existing code. For non-CUDA code the performance impact is likely to be much smaller, but still nice to have. Given that and the constraint of not wanting redundant pragma syntax, supporting the CUDA-style syntax ("#pragma unroll" and "#pragma unroll N") universally may be the way to go. At the very least the CUDA syntax should be supported in CUDA mode.</div>
<div><br></div><div>From a previous review Richard Smith (cc'd) had a strong opinion about placing the loop pragmas inside of the clang namespace ("#pragma clang loop..."). Clearly this is incompatible with "#pragma unroll ...". Richard, any thoughts on this?</div>
<div><br></div><div>For comparison, Intel and IBM compilers have the following syntax:</div><div><div><br></div><div>#pragma unroll</div><div>#pragma unroll(n)</div><div>#pragma nounroll</div></div><div><br></div><div>GCC and MSVC don't provide an unroll pragma, though unroll optimization parameters could be adjusted at a function-level granularity with "#pragma GCC optimize ..." or "#pragma optimize ..." respectively.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">I don't like how much duplication is happening in this code given that</blockquote>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">there's already existing machinery in place to do all of this.</blockquote>
<div><br></div><div> Once we determine what syntax we want to support I can work on reducing the redundancy in the code. Thanks for your comments.</div>
<div><br></div><div>Mark </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span><font color="#888888">
~Aaron<br>
</font></span><div><div><br>
><br>
> Mark<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>
> include/clang/Basic/DiagnosticSemaKinds.td<br>
> include/clang/Basic/TokenKinds.def<br>
> include/clang/Parse/Parser.h<br>
> include/clang/Sema/CudaUnrollHint.h<br>
> lib/CodeGen/CGStmt.cpp<br>
> lib/CodeGen/CodeGenFunction.h<br>
> lib/Parse/ParsePragma.cpp<br>
> lib/Parse/ParseStmt.cpp<br>
> lib/Sema/SemaStmtAttr.cpp<br>
> test/CodeGen/<a href="http://cuda-pragma-unroll.cu" target="_blank">cuda-pragma-unroll.cu</a><br>
> test/Misc/<a href="http://ast-print-cuda-pragmas.cu" target="_blank">ast-print-cuda-pragmas.cu</a><br>
> test/PCH/<a href="http://cuda-pragma-unroll.cu" target="_blank">cuda-pragma-unroll.cu</a><br>
> test/Parser/<a href="http://cuda-pragma-unroll.cu" target="_blank">cuda-pragma-unroll.cu</a><br>
</div></div></blockquote></div><br></div></div>