Re: Add 'aggressive' keyword for pragma loop hints

[Tyler Nowicki]

Hi Aaron, I made the changes you asked for. Here is the updated patch. Thanks for the review, Tyler

0001-Add-assume_safety-option-for-pragma-loop-vectorize-a.patch
Description: Binary data

On Jun 9, 2015, at 7:58 AM, Aaron Ballman <aa...@aaronballman.com> wrote: On Mon, Jun 8, 2015 at 6:05 PM, Tyler Nowicki <tnowi...@apple.com> 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
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits