| Message ID | 20151117114907.GA42296@msticlxl57.ims.intel.com |
|---|---|
| State | New |
| Headers | show |
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 (); > +}
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
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
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
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 >
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
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
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 (); +}