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
