[PATCH] Support constant expressions, including non-type template parameters, in pragma loop hints

[Aaron Ballman]

On Wed, Jul 30, 2014 at 1:33 PM, Tyler Nowicki <tnowicki at apple.com> wrote:
> Hi Aaron,
>
> Thanks for the review! Here is the updated patch and responses to some of
> your comments. The new tests are a couple of lines added to
> test/Parser/pragma-loop.cpp. This patch is just refactoring in preparation
> for new features so there really isn’t a need for new tests.

I only mentioned the tests because your original email had said "A
couple of new tests are included as well." But the latest patch has
your updated test, so I'm all set there.

>
> +                           ["default", "enable", "disable"],
> +                           ["Default", "Enable", "Disable"]>,
>
>
> This isn't really a "default" so much as "it's not Enabled or
> Disabled", correct? Perhaps UsesValue instead? I know that's not quite
> accurate either since it'll be used for #pragma unroll with no
> arguments…
>
>
> I’m pretty sure Default or something like it is correct. This value is used
> for #pragma unroll and #pragma nounroll. The default behavior with unroll
> enables the unroller and nounroll disables  it. UsesValue would imply that
> examining the state is required to determine if the Value parameter is used.
> What I would like to see in the future is for the state and value to be
> unioned. And the Option would determine if the state or value was needed.
> AFAIK Tablegen doesn’t have the ability to express unions right now.

You are correct that there's no way to express a union currently (it's
on my long list of things I'd like to do someday). Default will
suffice. :-)

>
>
> +    return true;
> +  }
> +
> +  bool StateOption = false;
> +  if (OptionInfo) // Pragma unroll does not specify an option.
>
>
> It doesn't always specify an option, but it sometimes does (otherwise
> there wouldn't be a .Case below).
>
>
> The naming of the pragmas is unfortunately a little confusing. #pragma
> unroll and #pragma nounroll don’t have options. There is no need because
> they only control unrolling. On the other hand #pragma clang loop has
> options which can be vectorize, interleave, unroll, etc. So when pragma
> unroll is used we don’t do the check that determines if the option has an
> argument for the state or value of the loop hint.
>
> #pragma unroll -> enables unrolling
> #pragma nounroll -> disables unrolling
> #pragma unroll(…) or #pragma unroll … -> specifies the amount of unrolling
>
> The first 2 cases are handled by the early test: if (!Info->HasValue &&
> (PragmaUnroll || PragmaNoUnroll)) { … return true; }
>
> After that we can always assume the 3rd case. So we leave StateOption false
> and drop down to the part that assumes the argument is a value.

That all makes sense to me, but it took a while for me to extrapolate
that from the code. I was only suggesting that the comment be updated,
but my suggestion was horribly obtuse.

Your changes LGTM! If you want to change the comment discussed above,
that's cool, but it's certainly not holding back the patch.

~Aaron