Add 'aggressive' keyword for pragma loop hints

[Tyler Nowicki]

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 <aaron at aaronballman.com> 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 <tnowicki at apple.com> 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 <aaron at aaronballman.com> wrote:
>> 
>> On Mon, Jun 8, 2015 at 6:05 PM, Tyler Nowicki <tnowicki at 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
>> 
>> 
>> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150611/60d42ce6/attachment.html>