<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=""><div class=""></div><div class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Jun 9, 2015, at 7:58 AM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com" class="">aaron@aaronballman.com</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="">On Mon, Jun 8, 2015 at 6:05 PM, Tyler Nowicki <</span><a href="mailto:tnowicki@apple.com" 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="">tnowicki@apple.com</a><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="">> wrote:</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="">Hi Aaron,<br class=""><br class="">Thanks for the review!<br class=""><br class="">Hal suggested a name change from ‘aggressive’ to ‘assume_safety’. The<br class="">attached patch has that change.<br class=""><br class=""><br class="">+ ["default", "aggressive", "enable", "disable"],<br class="">+ ["Default", "Aggressive", "Enable", "Disable"]>,<br class=""><br class=""><br class="">Entirely uncertain whether we care about this or not (we probably<br class="">don't), but this will mess up serialized data because it shifts the<br class="">option values around.<br class=""><br class=""><br class="">Oops, so if I put it at the end instead of the middle will that prevent<br class="">serialization problems?<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="">Yes, I believe it would.</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=""><br class="">- "'%select{enable|full}1' or 'disable'}0">;<br class="">+ "'%select{enable', 'aggressive|full}1' or 'disable'}0">;<br class=""><br class=""><br class="">The single quotes look strange here. I think in the case of selecting<br class="">enable, you will get two close single quotes, and in the case of<br class="">aggressive, you will get two open single quotes. I'm surprised a test<br class="">doesn't catch this, which suggests I may simply be wrong. ;-)<br class=""><br class=""><br class="">def err_pragma_loop_invalid_option : Error<<br class=""> "%select{invalid|missing}0 option%select{ %1|}0; expected vectorize, "<br class=""> "vectorize_width, interleave, interleave_count, unroll, or unroll_count">;<br class="">def err_pragma_invalid_keyword : Error<<br class="">- "invalid argument; expected '%select{enable|full}0' or 'disable'">;<br class="">+ "invalid argument; expected '%select{enable', 'aggressive|full}0' or<br class="">'disable'">;<br class=""><br class=""><br class="">Same problem here as above.<br class=""><br class=""><br class="">%select{} uses a | (pipe) between arguments. So there are two choices here<br class="">"enable’, ’assume_safety” and “full”. With the single quotes around the<br class="">select that results in either:<br class=""><br class="">‘enable’, ‘assume_safety’, or ‘disable’<br class="">and<br class="">‘full’, or ‘disable’<br class=""><br class="">If it is too confusing I could bring the single quotes into the select.<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="">Ugh, you are correct, I was misreading because of the commas. I don't</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="">have a strong opinion on whether the quotes should be inside the</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="">select or not, but I think it would make the diagnostic more readable</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="">for review purposes.</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="">~Aaron</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=""><br class="">// Pragma unroll support.<br class="">def warn_pragma_unroll_cuda_value_in_parens : Warning<<br class="">diff --git a/lib/CodeGen/CGLoopInfo.cpp b/lib/CodeGen/CGLoopInfo.cpp<br class="">index 011ae7e..c285092 100644<br class="">--- a/lib/CodeGen/CGLoopInfo.cpp<br class="">+++ b/lib/CodeGen/CGLoopInfo.cpp<br class="">@@ -8,13 +8,14 @@<br class="">//===----------------------------------------------------------------------===//<br class=""><br class="">#include "CGLoopInfo.h"<br class="">+#include "clang/AST/Attr.h"<br class="">+#include "clang/Sema/LoopHint.h"<br class="">#include "llvm/IR/BasicBlock.h"<br class="">#include "llvm/IR/Constants.h"<br class="">#include "llvm/IR/InstrTypes.h"<br class="">#include "llvm/IR/Instructions.h"<br class="">#include "llvm/IR/Metadata.h"<br class="">-using namespace clang;<br class="">-using namespace CodeGen;<br class="">+using namespace clang::CodeGen;<br class=""><br class=""><br class="">While I prefer this change, it does seem like a drive by that should<br class="">be in a separate commit.<br class=""><br class=""><br class="">Makes sense, I’ll commit it separately.<br class=""><br class=""><br class="">+ push(llvm::BasicBlock *Header,<br class="">+ llvm::ArrayRef<const Attr *> Attrs = llvm::ArrayRef<const Attr<br class="">*>());<br class=""><br class=""><br class="">Should use None here.<br class=""><br class=""><br class="">Done.<br class=""><br class="">- : !StateInfo->isStr("enable")) &&<br class="">+ : !StateInfo->isStr("enable") &&<br class="">+ !StateInfo->isStr("aggressive"))<br class="">&&<br class=""><br class=""><br class="">Indentation is a bit strange here, did clang-format do this?<br class=""><br class=""><br class="">Yes, I tried it twice just to make sure.<br class=""><br class="">Tyler</blockquote></div></blockquote></div><br class=""></div></body></html>