Add 'aggressive' keyword for pragma loop hints

[Aaron Ballman]

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
>
>
>
>