<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="">Thanks Aaron,</div><div class=""><br class=""></div><div class="">The patches are committed in <span style="font-family: 'Helvetica Neue';" class="">r239362, </span><span style="font-family: 'Helvetica Neue';" class="">r239363, </span><span style="font-family: 'Helvetica Neue';" class="">r239365, and r</span><span style="font-family: Menlo; font-size: 11px;" class="">239572.</span></div><div class=""><span style="font-family: 'Helvetica Neue';" class=""><br class=""></span></div><div class=""><font face="Helvetica Neue" class="">I’ll have a patch for the language extensions soon.</font></div><div class=""><font face="Helvetica Neue" class=""><br class=""></font></div><div class=""><font face="Helvetica Neue" class="">Tyler</font></div><div class=""><br class=""></div><br class=""><div><blockquote type="cite" class=""><div class="">On Jun 11, 2015, at 10:12 AM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com" class="">aaron@aaronballman.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class="">Changes LGTM with one request: please update LanguageExtensions.rst to<br class="">document the new functionality.<br class=""><br class="">Thanks!<br class=""><br class="">~Aaron<br class=""><br class="">On Wed, Jun 10, 2015 at 6:03 PM, Tyler Nowicki <<a href="mailto:tnowicki@apple.com" class="">tnowicki@apple.com</a>> wrote:<br class=""><blockquote type="cite" class="">Hi Aaron,<br class=""><br class="">I made the changes you asked for. Here is the updated patch.<br class=""><br class="">Thanks for the review,<br class=""><br class="">Tyler<br class=""><br class=""><br class=""><br class="">On Jun 9, 2015, at 7:58 AM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com" class="">aaron@aaronballman.com</a>> wrote:<br class=""><br class="">On Mon, Jun 8, 2015 at 6:05 PM, Tyler Nowicki <<a href="mailto:tnowicki@apple.com" class="">tnowicki@apple.com</a>> wrote:<br class=""><br 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=""><br class=""><br class="">Yes, I believe it would.<br class=""><br 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=""><br class=""><br class="">Ugh, you are correct, I was misreading because of the commas. I don't<br class="">have a strong opinion on whether the quotes should be inside the<br class="">select or not, but I think it would make the diagnostic more readable<br class="">for review purposes.<br class=""><br class="">~Aaron<br class=""><br 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<br class=""><br class=""><br class=""><br class=""></blockquote></div></blockquote></div><br class=""></body></html>