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