<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Sun, Jun 1, 2014 at 7:39 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"><div style="word-wrap:break-word"><div>Hi Richard,</div><div>
<br></div><div>Thanks for the detailed review!</div><div><br></div><div><div>I went with the last syntax you suggested. Seems simple to me and easily extendable: #pragma clang loop vectorize_width(2) interleave_count(4).</div>
<div><br></div><div>Also it helped clean up a lot of the ambiguity you talked about in the LoopHintAttr. A single integer Value has replaced the Kind (Enable/Disable/Value) and a integer value there previously. Hopefully it won’t be too difficult to support constant expressions, and non-type template parameters. I’ll work on those patches next.</div>
<div><br></div><div>I also improved the incompatibility testing as well. I realized there were several cases my previous test wasn’t identifying.</div><div><br></div><div><br></div></div><div><blockquote type="cite"><div style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>Objective C is a proper superset of C, so yes. I don't know Objective C very well either, and it might be the case that it would never be possible to vectorize its for loop, because it's too dynamic.</div>
</div></div></div></div></blockquote><div><br></div><div>Ok, its on my list.</div><div><br></div><div><br></div><blockquote type="cite"><div style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>I think you want (and what the approach I described above permits) is to callParseConstantExpression.</div></div></div></div></div></blockquote><div><br>
</div><div>I think I understand now what you are suggesting. Your thinking that the expression may include more than one token. I gave it a try, but I wasn’t able to get it to work. The expression returned by ParseConstantExpression was always invalid. Perhaps I didn’t implement it correctly. I’m capturing all of the tokens between the brackets and then when handling the annotated token I reinserted them into the stream and called ParseConstantExpression. I tried it with and without the brackets and with and without a trailing eof token. The LHS and Res expressions parsed in ParseConstantExpression were also always invalid <<NULL>>. Lets leave this and other constant expression work for a later patch.</div>
</div></div></blockquote><div><br></div><div>OK.</div><div><br></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">
<div style="word-wrap:break-word"><div>Here is the updated patch.</div></div></blockquote><div><br></div><div>I think this is very close to being ready.</div><div><br></div><div>+  let Args = [EnumArgument<"Option", "OptionType",<br>
</div><div><div>+                          ["vectorize", "vectorize_width", "interleave", "interleave_count"],</div><div>+                          ["Vectorize", "VectorizeWidth", "Interleave", "InterleaveCount"]>,</div>
<div>+              DefaultIntArgument<"Value", 1>];</div></div><div><br></div><div>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).</div>
<div><br></div><div>I also think we should have different spellings for these different enum values, once we have proper pragma spelling support:</div><div><br></div><div>  let Spellings = [Pragma<"clang", "loop", "vectorize_width">,</div>
<div>                   Pragma<"clang", "loop", "interleave_count">];</div><div>  let Args = [ExprArgument<"Value">];</div><div><br></div><div>... but that can all be deferred for a later patch.</div>
<div><br></div><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>+    if (ValueInt == 1 && (Option == LoopHintAttr::Vectorize ||</div><div>+                          Option == LoopHintAttr::Interleave)) {</div><div>+      Name = llvm::MDString::get(Context, "llvm.vectorizer.enable");</div>
<div>+      Value = Builder.getTrue();</div><div>+    } else {</div><div>+      if (ValueInt == 0)</div><div>+        ValueInt = 1; // Vectorization/interleaving is disabled.</div><div>+      Name = llvm::MDString::get(Context, MetadataNames[Option]);</div>
<div>+      Value = llvm::ConstantInt::get(Int32Ty, ValueInt);</div><div>+    }</div><div><br></div><div>Please change this to a 'switch' to make this code more robust against enumerators being added later.<br></div>
<div><br></div><div><br></div><div><div>+///  loop-hint:</div><div>+///    'vectorize' '(' loop-hint-value ')'</div><div>+///    'interleave' '(' loop-hint-value ')'</div><div>
+///</div><div>+///  loop-hint-value:</div><div>+///    'enable'</div><div>+///    'disable'</div><div>+///    constant-expression</div></div><div><br></div><div>This needs to be updated.</div><div><br></div>
<div><br></div><div><div>+// CHECK: br i1 %cmp, label %while.body, label %while.end, !llvm.loop !1</div><div>+// CHECK: br i1 %cmp, label %do.body, label %do.end, !llvm.loop !5</div><div>+// CHECK: br i1 %cmp, label %for.body, label %for.end, !llvm.loop !7</div>
<div>+// CHECK: br i1 %cmp, label %for.body, label %for.end, !llvm.loop !8</div><div>+// CHECK: br i1 %cmp, label %for.body, label %for.end, !llvm.loop !11</div><div>+// CHECK: br i1 %cmp, label %for.body, label %for.end, !llvm.loop !13</div>
<div>+// CHECK: br i1 %cmp, label %for.body, label %for.end, !llvm.loop !14</div><div>+// CHECK: br i1 %cmp, label %for.body, label %for.end, !llvm.loop !16</div></div><div><br></div><div>I believe these tests will fail on a NDEBUG build, because we don't give names to values and basic blocks there. What we'd usually do is something like this:</div>
<div><br></div><div><div>+void while_test(int *List, int Length) {</div><div>+  // CHECK: define {{.*}} @_Z10while_test</div><div>+  int i = 0;</div><div>+</div><div>+#pragma clang loop vectorize(enable)</div><div>+#pragma clang loop interleave_count(4)</div>
<div>+#pragma clang loop vectorize_width(4)</div><div>+  while (i < Length) {</div><div>+    // CHECK: br i1 {{.*}}, label {{.*}}, label {{.*}}, !llvm.loop ![[LOOP_1:.*]]</div><div>+    List[i] = i * 2;</div><div>+    i++;</div>
<div>+  }</div><div>+}</div></div><div>[... at end of file ...]</div><div>+// CHECK: ![[LOOP_1]] = metadata !{metadata ![[LOOP_1]], metadata ![[WIDTH_4:.*]], metadata ![[UNROLL_4:.*]], metadata ![[ENABLE_1:.*]]}</div><div>
+// CHECK: ![[WIDTH_4]] = metadata !{metadata !"llvm.vectorizer.width", i32 4}</div><div>[...]</div></div></div></div>