<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Jun 2, 2014 at 4:18 PM, Tyler Nowicki <span dir="ltr"><<a href="mailto:tnowicki@apple.com" target="_blank">tnowicki@apple.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">Hi Richard,<br>
<br>
Thanks again for the review. I’m glad to hear its getting closer. I’ve applied your suggestions. Here is the updated patch.<br>
<div class=""><br>
> Once you move to supporting arbitrary constant expressions as the argument, I think you may want to split this into two separate Attrs (one for vectorize and interleave, that have a BoolArgument, and one for vectorize_width and interleave_count, that have an ExprArgument).<br>

><br>
> I also think we should have different spellings for these different enum values, once we have proper pragma spelling support:<br>
><br>
>   let Spellings = [Pragma<"clang", "loop", "vectorize_width">,<br>
>                    Pragma<"clang", "loop", "interleave_count">];<br>
>   let Args = [ExprArgument<"Value">];<br>
><br>
> ... but that can all be deferred for a later patch.<br>
<br>
</div>I like the idea of separating the LoopHintAttrs that may make it easier to support c++11 spellings as well.</blockquote><div><br></div><div>+    const char *MetadataNames[] = {</div><div>+        "llvm.vectorizer.width", "llvm.vectorizer.width",</div>
<div>+        "llvm.vectorizer.unroll", "llvm.vectorizer.unroll"};</div><div>+</div><div>+    switch (Option) {</div><div>+    case LoopHintAttr::Vectorize:</div><div>+    case LoopHintAttr::Interleave:</div>
<div><br></div><div>Please remove the array and set the strings inside the switch, so you're not depending on the values of the enumerators here.</div><div><br></div><div>Other than that, LGTM; thank you for your patience!</div>
</div></div></div>