<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="">Thanks!<div class=""><br class=""></div><div class="">Committed in r243315.</div><div class=""><br class=""></div><div class="">Tyler</div><div class=""><font face="Menlo" class=""><span style="font-size: 11px;" class=""><br class=""></span></font><div style=""><blockquote type="cite" class=""><div class="">On Jul 26, 2015, at 4:27 AM, Hal Finkel <<a href="mailto:hfinkel@anl.gov" class="">hfinkel@anl.gov</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">----- Original Message -----</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">From: "Tyler Nowicki" <<a href="mailto:tnowicki@apple.com" class="">tnowicki@apple.com</a>><br class="">To: "Hal Finkel" <<a href="mailto:hfinkel@anl.gov" class="">hfinkel@anl.gov</a>><br class="">Cc: "llvm cfe" <<a href="mailto:cfe-commits@cs.uiuc.edu" class="">cfe-commits@cs.uiuc.edu</a>>, "Gerolf Hoflehner" <<a href="mailto:ghoflehner@apple.com" class="">ghoflehner@apple.com</a>><br class="">Sent: Monday, July 20, 2015 1:06:28 PM<br class="">Subject: Re: [Patch][LoopInfo] Use CGLoopInfo to generate loop hints<br class=""><br class="">Hi,<br class=""><br class="">Could I get a review of the latest patch?<br class=""></blockquote><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">This LGTM, thanks!</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">-Hal</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br class="">Thanks,<br class=""><br class=""><br class="">Tyler<br class=""><br class=""><br class="">On Jul 14, 2015, at 4:45 PM, Tyler Nowicki < <a href="mailto:tnowicki@apple.com" class="">tnowicki@apple.com</a> ><br class="">wrote:<br class=""><br class="">Hi Hal,<br class=""><br class=""><br class="">Thanks for the review! I have committed the renaming patch.<br class=""><br class=""><br class="">Comments in-line and I’ve attached an updated patch for review<br class=""><br class=""><br class=""><br class=""><br class=""><br class=""><br class="">On Jul 9, 2015, at 6:12 PM, Hal Finkel < <a href="mailto:hfinkel@anl.gov" class="">hfinkel@anl.gov</a> > wrote:<br class=""><br 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<br class="">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=""><br class=""><br class=""><br class="">Removed.<br class=""><br class=""><br class=""><br class=""><br class=""><br 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<br class="">or case LoopHintAttr::Enable seems counter-intuitive. If this is<br class="">right, please add a comment explaining why.<br class=""><br class=""><br class=""><br class="">Specifying something like vectorizea_width(enable/disable) is invalid<br class="">and produces an error. Perhaps we could improve on the message:<br class="">error: use of undeclared identifier ‘disable’. So the case<br class="">statements should never be triggered. I’ll add unreachable() calls<br class="">to make sure that is clear.<br class=""><br class=""><br class=""><br class=""><br class=""><br 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<br class="">![[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<br class="">different branch?<br class=""><br class=""><br class=""><br class="">Yes, that is part of the reason for this change.<br class=""><br class=""><br class=""><br class=""><br class=""><br class=""><br class=""><br class=""><br class=""><br class=""><br class=""><br class=""><br class="">"I noticed a problem with my implementation of #pragma clang loop<br class=""><br class=""><br class=""><br class=""><br class=""><br class=""><br class=""><br class=""><br class=""><br class=""><br class="">vectorize(assume_safety). When it was specified on a loop other<br class=""><br class=""><br class=""><br class=""><br class=""><br class=""><br class=""><br class=""><br class=""><br class=""><br class="">loop hints were lost. The problem is that CGLoopInfo attaches<br class=""><br class=""><br class=""><br class=""><br class=""><br class=""><br class=""><br class=""><br class=""><br class=""><br class="">metadata a little bit differently than EmitCondBrHints in CGStmt.<br class=""><br class=""><br class=""><br class=""><br class=""><br class=""><br class=""><br class=""><br class=""><br class=""><br class="">For do-loops CGLoopInfo attaches metadata to the br in the body<br class=""><br class=""><br class=""><br class=""><br class=""><br class=""><br class=""><br class=""><br class=""><br class=""><br class="">block and for while and for loops, the inc block. EmitCondBrHints<br class=""><br class=""><br class=""><br class=""><br class=""><br class=""><br class=""><br class=""><br class=""><br class=""><br class="">on the other hand always attaches data to the br in the cond<br class=""><br class=""><br class=""><br class=""><br class=""><br class=""><br class=""><br class=""><br class=""><br class=""><br class="">block. When specifying assume_safety CGLoopInfo emits an empty<br class=""><br class=""><br class=""><br class=""><br class=""><br class=""><br class=""><br class=""><br class=""><br class=""><br class="">llvm.loop metadata shadowing the metadata in the cond block. Loop<br class=""><br class=""><br class=""><br class=""><br class=""><br class=""><br class=""><br class=""><br class=""><br class=""><br class="">transformations like rotate and unswitch would then eliminate the<br class="">cond block.”<br class=""><br class=""><br class="">So when vectorize(assume_safety) is specified EmitCondBrHints would<br class="">add the loop hint vectorize.enable = true to the cond br. But that<br class="">would be lost by loop transformations. There certainly are other<br class="">methods of fixing this problem, but I think unifying how metadata is<br class="">attached makes the most sense and is the best for llvm in the long<br class="">term.<br class=""><br class=""><br class="">I will post a patch for documentation of assume_safety once this<br class="">problem has been fixed.<br class=""><br class=""><br class="">Tyler<br class=""><br class=""><0001-Moved-functionality-from-EmitCondBr-to-CGLoopInfo.patch><br class=""><br class=""></blockquote><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">--<span class="Apple-converted-space"> </span></span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">Hal Finkel</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">Assistant Computational Scientist</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">Leadership Computing Facility</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">Argonne National Laboratory</span></div></blockquote></div><br class=""></div></body></html>