Add 'aggressive' keyword for pragma loop hints

[Tyler Nowicki]

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?


>> -  "'%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.


>> // 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/20150608/100c88d1/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-Add-assume_safety-option-for-pragma-loop-vectorize-a.patch
Type: application/octet-stream
Size: 18573 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150608/100c88d1/attachment.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150608/100c88d1/attachment-0001.html>