<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="">Hi Hal,<div class=""><br class=""></div><div class="">Thanks for the review! I have committed the renaming patch.</div><div class=""><br class=""></div><div class="">Comments in-line and I’ve attached an updated patch for review</div><div class=""><br class=""></div><div class=""><div><blockquote type="cite" class=""><div class="">On Jul 9, 2015, at 6:12 PM, Hal Finkel <<a href="mailto:hfinkel@anl.gov" class="">hfinkel@anl.gov</a>> wrote:</div><br class="Apple-interchange-newline"><div class="">Hi Tyler,<br class=""><br class="">0001-Rename-Vectorizer-to-Vectorize-and-VectorizeUnroll-t.patch:<br class=""><br class=""> This makes sense; it makes the variable names match the name of the metadata they control. LGTM.<br class=""><br class="">0002-Moved-functionality-from-EmitCondBr-to-CGLoopInfo.patch:<br class=""><br class="">+ ValueInt = static_cast<unsigned>(ValueAPS.getSExtValue());<br class=""></div></blockquote><div><br class=""></div><div>Removed.</div><div><br class=""></div><br class=""><blockquote type="cite" class=""><div class="">Do Clang or GCC warn here without the static cast? If not, remove it.<br class=""><br class="">+ case LoopHintAttr::UnrollCount:<br class="">+ case LoopHintAttr::VectorizeWidth:<br class="">+ case LoopHintAttr::InterleaveCount:<br class="">+ break;<br class=""><br class="">The fact that there is no code here for either LoopHintAttr::Disable or case LoopHintAttr::Enable seems counter-intuitive. If this is right, please add a comment explaining why.<br class=""></div></blockquote><div><br class=""></div><div>Specifying something like vectorizea_width(enable/disable) is invalid and produces an error. Perhaps we could improve on the message: <span style="color: rgb(195, 55, 32);" class=""><b class="">error: </b></span><b class="">use of undeclared identifier ‘disable’. </b>So the case statements should never be triggered. I’ll add unreachable() calls to make sure that is clear.</div><div><br class=""></div><br class=""><blockquote type="cite" class=""><div class="">--- a/test/CodeGenCXX/pragma-loop.cpp<br class="">+++ b/test/CodeGenCXX/pragma-loop.cpp<br class="">@@ -10,7 +10,7 @@ void while_test(int *List, int Length) {<br class=""> #pragma clang loop vectorize_width(4)<br class=""> #pragma clang loop unroll(full)<br class=""> while (i < Length) {<br class="">- // CHECK: br i1 {{.*}}, label {{.*}}, label {{.*}}, !llvm.loop ![[LOOP_1:.*]]<br class="">+ // CHECK: br label {{.*}}, !llvm.loop ![[LOOP_1:.*]]<br class=""> List[i] = i * 2;<br class=""> i++;<br class=""> }<br class=""><br class="">Why are these things changing? Are we now attaching the metadata to a different branch?<br class=""></div></blockquote><div><br class=""></div><div>Yes, that is part of the reason for this change. </div><div><br class=""></div><div><blockquote type="cite" class=""><blockquote type="cite" class=""><blockquote type="cite" class=""><blockquote type="cite" class=""><blockquote type="cite" class=""></blockquote></blockquote></blockquote></blockquote></blockquote>"I noticed a problem with my implementation of #pragma clang loop<br class=""><blockquote type="cite" class=""><blockquote type="cite" class=""><blockquote type="cite" class=""><blockquote type="cite" class=""><blockquote type="cite" class=""></blockquote></blockquote></blockquote></blockquote></blockquote>vectorize(assume_safety). When it was specified on a loop other<br class=""><blockquote type="cite" class=""><blockquote type="cite" class=""><blockquote type="cite" class=""><blockquote type="cite" class=""><blockquote type="cite" class=""></blockquote></blockquote></blockquote></blockquote></blockquote>loop hints were lost. The problem is that CGLoopInfo attaches<br class=""><blockquote type="cite" class=""><blockquote type="cite" class=""><blockquote type="cite" class=""><blockquote type="cite" class=""><blockquote type="cite" class=""></blockquote></blockquote></blockquote></blockquote></blockquote>metadata a little bit differently than EmitCondBrHints in CGStmt.<br class=""><blockquote type="cite" class=""><blockquote type="cite" class=""><blockquote type="cite" class=""><blockquote type="cite" class=""><blockquote type="cite" class=""></blockquote></blockquote></blockquote></blockquote></blockquote>For do-loops CGLoopInfo attaches metadata to the br in the body<br class=""><blockquote type="cite" class=""><blockquote type="cite" class=""><blockquote type="cite" class=""><blockquote type="cite" class=""><blockquote type="cite" class=""></blockquote></blockquote></blockquote></blockquote></blockquote>block and for while and for loops, the inc block. EmitCondBrHints<br class=""><blockquote type="cite" class=""><blockquote type="cite" class=""><blockquote type="cite" class=""><blockquote type="cite" class=""><blockquote type="cite" class=""></blockquote></blockquote></blockquote></blockquote></blockquote>on the other hand always attaches data to the br in the cond<br class=""><blockquote type="cite" class=""><blockquote type="cite" class=""><blockquote type="cite" class=""><blockquote type="cite" class=""><blockquote type="cite" class=""></blockquote></blockquote></blockquote></blockquote></blockquote>block. When specifying assume_safety CGLoopInfo emits an empty<br class=""><blockquote type="cite" class=""><blockquote type="cite" class=""><blockquote type="cite" class=""><blockquote type="cite" class=""><blockquote type="cite" class=""></blockquote></blockquote></blockquote></blockquote></blockquote>llvm.loop metadata shadowing the metadata in the cond block. Loop<br class=""><blockquote type="cite" class=""><blockquote type="cite" class=""><blockquote type="cite" class=""><blockquote type="cite" class=""><blockquote type="cite" class=""></blockquote></blockquote></blockquote></blockquote></blockquote>transformations like rotate and unswitch would then eliminate the<br class="">cond block.”</div><div><br class=""></div><div>So when vectorize(assume_safety) is specified EmitCondBrHints would add the loop hint vectorize.enable = true to the cond br. But that would be lost by loop transformations. There certainly are other methods of fixing this problem, but I think unifying how metadata is attached makes the most sense and is the best for llvm in the long term.</div><div><br class=""></div><div>I will post a patch for documentation of assume_safety once this problem has been fixed.</div><div><br class=""></div><div>Tyler</div><div><br class=""></div></div></div></body></html>