[PATCH, PR middle-end/68134] Reject scalar modes in default get_mask_mode hook

[PATCH, PR middle-end/68134] Reject scalar modes in default get_mask_mode hook

[Ilya Enkovich]

Hi,

Default hook for get_mask_mode is supposed to return integer vector modes.  This means it should reject calar modes returned by mode_for_vector.  Bootstrapped and regtested on x86_64-unknown-linux-gnu, regtested on aarch64-unknown-linux-gnu.  OK for trunk?

Thanks,
Ilya
--
gcc/

2015-11-17  Ilya Enkovich  <enkovich.gnu@gmail.com>

	PR middle-end/68134
	* targhooks.c (default_get_mask_mode): Filter out
	scalar modes returned by mode_for_vector.

gcc/testsuite/

2015-11-17  Ilya Enkovich  <enkovich.gnu@gmail.com>

	PR middle-end/68134
	* gcc.dg/pr68134.c: New test.


diff --git a/gcc/targhooks.c b/gcc/targhooks.c
index c34b4e9..66d983b 100644
--- a/gcc/targhooks.c
+++ b/gcc/targhooks.c
@@ -1093,8 +1093,8 @@ default_get_mask_mode (unsigned nunits, unsigned vector_size)
   gcc_assert (elem_size * nunits == vector_size);
 
   vector_mode = mode_for_vector (elem_mode, nunits);
-  if (VECTOR_MODE_P (vector_mode)
-      && !targetm.vector_mode_supported_p (vector_mode))
+  if (!VECTOR_MODE_P (vector_mode)
+      || !targetm.vector_mode_supported_p (vector_mode))
     vector_mode = BLKmode;
 
   return vector_mode;
diff --git a/gcc/testsuite/gcc.dg/pr68134.c b/gcc/testsuite/gcc.dg/pr68134.c
new file mode 100644
index 0000000..522b4c6
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr68134.c
@@ -0,0 +1,19 @@
+/* { dg-do compile } */
+/* { dg-options "-std=c99" } */
+
+#include <stdint.h>
+
+typedef double float64x1_t __attribute__ ((vector_size (8)));
+typedef uint64_t uint64x1_t;
+
+void
+foo (void)
+{
+  float64x1_t arg1 = (float64x1_t) 0x3fedf9d4343c7c80;
+  float64x1_t arg2 = (float64x1_t) 0x3fcdc53742ea9c40;
+  uint64x1_t result = (uint64x1_t) (arg1 == arg2);
+  uint64_t got = result;
+  uint64_t exp = 0;
+  if (got != 0)
+    __builtin_abort ();
+}

Re: [PATCH, PR middle-end/68134] Reject scalar modes in default get_mask_mode hook

[Richard Biener]

On Tue, Nov 17, 2015 at 12:49 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
> Hi,
>
> Default hook for get_mask_mode is supposed to return integer vector modes.  This means it should reject calar modes returned by mode_for_vector.  Bootstrapped and regtested on x86_64-unknown-linux-gnu, regtested on aarch64-unknown-linux-gnu.  OK for trunk?

Ok.

Thanks,
Richard.

> Thanks,
> Ilya
> --
> gcc/
>
> 2015-11-17  Ilya Enkovich  <enkovich.gnu@gmail.com>
>
>         PR middle-end/68134
>         * targhooks.c (default_get_mask_mode): Filter out
>         scalar modes returned by mode_for_vector.
>
> gcc/testsuite/
>
> 2015-11-17  Ilya Enkovich  <enkovich.gnu@gmail.com>
>
>         PR middle-end/68134
>         * gcc.dg/pr68134.c: New test.
>
>
> diff --git a/gcc/targhooks.c b/gcc/targhooks.c
> index c34b4e9..66d983b 100644
> --- a/gcc/targhooks.c
> +++ b/gcc/targhooks.c
> @@ -1093,8 +1093,8 @@ default_get_mask_mode (unsigned nunits, unsigned vector_size)
>    gcc_assert (elem_size * nunits == vector_size);
>
>    vector_mode = mode_for_vector (elem_mode, nunits);
> -  if (VECTOR_MODE_P (vector_mode)
> -      && !targetm.vector_mode_supported_p (vector_mode))
> +  if (!VECTOR_MODE_P (vector_mode)
> +      || !targetm.vector_mode_supported_p (vector_mode))
>      vector_mode = BLKmode;
>
>    return vector_mode;
> diff --git a/gcc/testsuite/gcc.dg/pr68134.c b/gcc/testsuite/gcc.dg/pr68134.c
> new file mode 100644
> index 0000000..522b4c6
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/pr68134.c
> @@ -0,0 +1,19 @@
> +/* { dg-do compile } */
> +/* { dg-options "-std=c99" } */
> +
> +#include <stdint.h>
> +
> +typedef double float64x1_t __attribute__ ((vector_size (8)));
> +typedef uint64_t uint64x1_t;
> +
> +void
> +foo (void)
> +{
> +  float64x1_t arg1 = (float64x1_t) 0x3fedf9d4343c7c80;
> +  float64x1_t arg2 = (float64x1_t) 0x3fcdc53742ea9c40;
> +  uint64x1_t result = (uint64x1_t) (arg1 == arg2);
> +  uint64_t got = result;
> +  uint64_t exp = 0;
> +  if (got != 0)
> +    __builtin_abort ();
> +}

Re: [PATCH, PR middle-end/68134] Reject scalar modes in default get_mask_mode hook

[Bernd Schmidt]

On 11/17/2015 12:49 PM, Ilya Enkovich wrote:
> Default hook for get_mask_mode is supposed to return integer vector
> modes.  This means it should reject calar modes returned by
> mode_for_vector.  Bootstrapped and regtested on
> x86_64-unknown-linux-gnu, regtested on aarch64-unknown-linux-gnu.  OK
> for trunk?

You didn't say what exactly fails if an integer mode is returned. I'm 
assuming it's build_truth_vector_type which can call make_vector_type 
with an integer mode.

The patch looks OK to me.


Bernd

Re: [PATCH, PR middle-end/68134] Reject scalar modes in default get_mask_mode hook

[Ilya Enkovich]

2015-11-17 15:26 GMT+03:00 Bernd Schmidt <bschmidt@redhat.com>:
> On 11/17/2015 12:49 PM, Ilya Enkovich wrote:
>>
>> Default hook for get_mask_mode is supposed to return integer vector
>> modes.  This means it should reject calar modes returned by
>> mode_for_vector.  Bootstrapped and regtested on
>> x86_64-unknown-linux-gnu, regtested on aarch64-unknown-linux-gnu.  OK
>> for trunk?
>
>
> You didn't say what exactly fails if an integer mode is returned. I'm
> assuming it's build_truth_vector_type which can call make_vector_type with
> an integer mode.

In case of integer mode we don't have such instruction in optab but
don't lower it either.

Ilya

>
> The patch looks OK to me.
>
>
> Bernd

Re: [PATCH, PR middle-end/68134] Reject scalar modes in default get_mask_mode hook

[Alan Lawrence]

On 17/11/15 11:49, Ilya Enkovich wrote:
> Hi,
>
> Default hook for get_mask_mode is supposed to return integer vector modes.  This means it should reject calar modes returned by mode_for_vector.  Bootstrapped and regtested on x86_64-unknown-linux-gnu, regtested on aarch64-unknown-linux-gnu.  OK for trunk?
>
> Thanks,
> Ilya
> --
> gcc/
>
> 2015-11-17  Ilya Enkovich  <enkovich.gnu@gmail.com>
>
> 	PR middle-end/68134
> 	* targhooks.c (default_get_mask_mode): Filter out
> 	scalar modes returned by mode_for_vector.

I've been playing around with a patch that expands arbitrary VEC_COND_EXPRs 
using the vec_cmp and vcond_mask optabs - allowing platforms to drop the ugly 
vcond<mode><mode> pattern (for which a cross-product of modes is usually 
required) where vcond = vec_cmp + vcond_mask. (E.g. ARM and AArch64.)

Mostly this is fairly straightforward, relatively little midend code is 
required, and the backend cleans up quite a bit. However, I get stuck on the 
case of singleton vectors (64x1). No surprises there, then...

The PR/68134 fix, makes the 'mask mode' for comparing 64x1 vectors, into 
BLKmode, so that we get past this in expand_vector_operations:

/* A scalar operation pretending to be a vector one.  */
   if (VECTOR_BOOLEAN_TYPE_P (type)
       && !VECTOR_MODE_P (TYPE_MODE (type))
       && TYPE_MODE (type) != BLKmode)
     return;

and we do the operation piecewise. (Which is what we want; there is only one piece!)

However, with my vec_cmp + vcond_mask changes, dropping vconddidi, this means we 
look for a vcond_maskdiblk and vec_cmpdiblk. Which doesn't really feel right - 
it feels like the 64x1 mask should be a DImode, just like other 64x1 vectors.

So, I'm left thinking - is there a better way to look for "scalar operations 
pretending to be vector ones", such that we can get a DImode vec<bool> through 
the above? What exactly do we want that check to do? In my AArch64 testing, I 
was able to take it out altogether and get all tests passing...? (I don't have 
AVX512 hardware)

Thanks, Alan

Re: [PATCH, PR middle-end/68134] Reject scalar modes in default get_mask_mode hook

[Ilya Enkovich]

2016-02-19 20:36 GMT+03:00 Alan Lawrence <alan.lawrence@foss.arm.com>:
> On 17/11/15 11:49, Ilya Enkovich wrote:
>>
>> Hi,
>>
>> Default hook for get_mask_mode is supposed to return integer vector modes.
>> This means it should reject calar modes returned by mode_for_vector.
>> Bootstrapped and regtested on x86_64-unknown-linux-gnu, regtested on
>> aarch64-unknown-linux-gnu.  OK for trunk?
>>
>> Thanks,
>> Ilya
>> --
>> gcc/
>>
>> 2015-11-17  Ilya Enkovich  <enkovich.gnu@gmail.com>
>>
>>         PR middle-end/68134
>>         * targhooks.c (default_get_mask_mode): Filter out
>>         scalar modes returned by mode_for_vector.
>
>
> I've been playing around with a patch that expands arbitrary VEC_COND_EXPRs
> using the vec_cmp and vcond_mask optabs - allowing platforms to drop the
> ugly vcond<mode><mode> pattern (for which a cross-product of modes is
> usually required) where vcond = vec_cmp + vcond_mask. (E.g. ARM and
> AArch64.)
>
> Mostly this is fairly straightforward, relatively little midend code is
> required, and the backend cleans up quite a bit. However, I get stuck on the
> case of singleton vectors (64x1). No surprises there, then...
>
> The PR/68134 fix, makes the 'mask mode' for comparing 64x1 vectors, into
> BLKmode, so that we get past this in expand_vector_operations:
>
> /* A scalar operation pretending to be a vector one.  */
>   if (VECTOR_BOOLEAN_TYPE_P (type)
>       && !VECTOR_MODE_P (TYPE_MODE (type))
>       && TYPE_MODE (type) != BLKmode)
>     return;
>
> and we do the operation piecewise. (Which is what we want; there is only one
> piece!)
>
> However, with my vec_cmp + vcond_mask changes, dropping vconddidi, this
> means we look for a vcond_maskdiblk and vec_cmpdiblk. Which doesn't really
> feel right - it feels like the 64x1 mask should be a DImode, just like other
> 64x1 vectors.

The problem here is to distinguish vector mask of one DI element and
DI scalar mask.  We don't want to lower scalar mask manipulations
because they are simple integer operations, not vector ones. Probably
vector of a single DI should have V1DI mode and not pretend to be a
scalar?  This would make things easier.

>
> So, I'm left thinking - is there a better way to look for "scalar operations
> pretending to be vector ones", such that we can get a DImode vec<bool>
> through the above? What exactly do we want that check to do? In my AArch64
> testing, I was able to take it out altogether and get all tests passing...?
> (I don't have AVX512 hardware)

You were able to do it because it is related to scalar masks supported
by AVX512 only for now.

Thanks,
Ilya

>
> Thanks, Alan
>

Re: [PATCH, PR middle-end/68134] Reject scalar modes in default get_mask_mode hook

[Alan Lawrence]

On 20/02/16 09:29, Ilya Enkovich wrote:
> 2016-02-19 20:36 GMT+03:00 Alan Lawrence <alan.lawrence@foss.arm.com>:
>> Mostly this is fairly straightforward, relatively little midend code is
>> required, and the backend cleans up quite a bit. However, I get stuck on the
>> case of singleton vectors (64x1). No surprises there, then...
>>
>> The PR/68134 fix, makes the 'mask mode' for comparing 64x1 vectors, into
>> BLKmode, so that we get past this in expand_vector_operations:
>>
>> /* A scalar operation pretending to be a vector one.  */
>>    if (VECTOR_BOOLEAN_TYPE_P (type)
>>        && !VECTOR_MODE_P (TYPE_MODE (type))
>>        && TYPE_MODE (type) != BLKmode)
>>      return;
>>
>> and we do the operation piecewise. (Which is what we want; there is only one
>> piece!)
>>
>> However, with my vec_cmp + vcond_mask changes, dropping vconddidi, this
>> means we look for a vcond_maskdiblk and vec_cmpdiblk. Which doesn't really
>> feel right - it feels like the 64x1 mask should be a DImode, just like other
>> 64x1 vectors.
>
> The problem here is to distinguish vector mask of one DI element and
> DI scalar mask.  We don't want to lower scalar mask manipulations
> because they are simple integer operations, not vector ones. Probably
> vector of a single DI should have V1DI mode and not pretend to be a
> scalar?  This would make things easier.

Thanks for the quick reply, Ilya.

What's the difference between, as you say, a "simple integer operation" and a 
"vector" operation of just one element?

This is why we do *not* have V1DImode in the AArch64 (or ARM) backends, but 
instead treat 64x1 vectors as DImode - the operations are the same; so keeping 
them as the same mode, enables CSE and lots of other optimizations, plus we 
don't have to have two near-identical copies (DI + V1DI) for many patterns, etc...

If the operations were on a "DI scalar mask", when would the first part of that 
test, VECTOR_BOOLEAN_TYPE_P, hold?

Thanks, Alan

Re: [PATCH, PR middle-end/68134] Reject scalar modes in default get_mask_mode hook

[Ilya Enkovich]

2016-02-22 14:50 GMT+03:00 Alan Lawrence <alan.lawrence@foss.arm.com>:
> On 20/02/16 09:29, Ilya Enkovich wrote:
>>
>> 2016-02-19 20:36 GMT+03:00 Alan Lawrence <alan.lawrence@foss.arm.com>:
>>>
>>> On 17/11/15 11:49, Ilya Enkovich wrote:
>>>>
>>>>
>>>> Hi,
>>>>
>>>> Default hook for get_mask_mode is supposed to return integer vector
>>>> modes.
>>>> This means it should reject calar modes returned by mode_for_vector.
>>>> Bootstrapped and regtested on x86_64-unknown-linux-gnu, regtested on
>>>> aarch64-unknown-linux-gnu.  OK for trunk?
>>>>
>>>> Thanks,
>>>> Ilya
>>>> --
>>>> gcc/
>>>>
>>>> 2015-11-17  Ilya Enkovich  <enkovich.gnu@gmail.com>
>>>>
>>>>          PR middle-end/68134
>>>>          * targhooks.c (default_get_mask_mode): Filter out
>>>>          scalar modes returned by mode_for_vector.
>>>
>>>
>>>
>>> I've been playing around with a patch that expands arbitrary
>>> VEC_COND_EXPRs
>>> using the vec_cmp and vcond_mask optabs - allowing platforms to drop the
>>> ugly vcond<mode><mode> pattern (for which a cross-product of modes is
>>> usually required) where vcond = vec_cmp + vcond_mask. (E.g. ARM and
>>> AArch64.)
>>>
>>> Mostly this is fairly straightforward, relatively little midend code is
>>> required, and the backend cleans up quite a bit. However, I get stuck on
>>> the
>>> case of singleton vectors (64x1). No surprises there, then...
>>>
>>> The PR/68134 fix, makes the 'mask mode' for comparing 64x1 vectors, into
>>> BLKmode, so that we get past this in expand_vector_operations:
>>>
>>> /* A scalar operation pretending to be a vector one.  */
>>>    if (VECTOR_BOOLEAN_TYPE_P (type)
>>>        && !VECTOR_MODE_P (TYPE_MODE (type))
>>>        && TYPE_MODE (type) != BLKmode)
>>>      return;
>>>
>>> and we do the operation piecewise. (Which is what we want; there is only
>>> one
>>> piece!)
>>>
>>> However, with my vec_cmp + vcond_mask changes, dropping vconddidi, this
>>> means we look for a vcond_maskdiblk and vec_cmpdiblk. Which doesn't
>>> really
>>> feel right - it feels like the 64x1 mask should be a DImode, just like
>>> other
>>> 64x1 vectors.
>>
>>
>> The problem here is to distinguish vector mask of one DI element and
>> DI scalar mask.  We don't want to lower scalar mask manipulations
>> because they are simple integer operations, not vector ones. Probably
>> vector of a single DI should have V1DI mode and not pretend to be a
>> scalar?  This would make things easier.
>
>
> Thanks for the quite reply, Ilya.
>
> What's the difference between, as you say, a "simple integer operation" and
> a "vector" operation of just one element?

The difference is at least in how this operation is expanded.  You
would use different optabs for scalar and vector cases. Also note that
default_get_mask_mode uses BLKmode for scalar modes not just because
of a single element vector.  mode_for_vector may return DImode for
V2SI and V4HI vectors in case target doesn't define such vector modes.
To distinguish true scalar masks I avoid scalar mode usage for
non-scalar masks.  One element vector might be an exception here.  You
may try to define TARGET_VECTORIZE_GET_MASK_MODE for your target and
keep scalar mode when you want it.

>
> This is why we do *not* have V1DImode in the AArch64 (or ARM) backends, but
> instead treat 64x1 vectors as DImode - the operations are the same; so
> keeping them as the same mode, enables CSE and lots of other optimizations,
> plus we don't have to have two near-identical copies (DI + V1DI) for many
> patterns, etc...

Well, you don't have to keep V1DI mode after expand.  You may also
just don't add any new patterns for vector optabs and therefore get
these vector operations lowered into 'true' scalar operations with
both scalar type and mode.

>
> If the operations were on a "DI scalar mask", when would the first part of
> that test, VECTOR_BOOLEAN_TYPE_P, hold?

Any vector comparison produces a value of a boolean vector type.  Even
if these vectors and mask have a scalar mode.

Thanks,
Ilya

>
> Thanks, Alan