Re: [PATCH] Retain TYPE_MODE more often for BIT_FIELD_REFs in get_inner_referece
> 2019-09-26 Richard Biener > > PR middle-end/91897 > * expr.c (get_inner_reference): For BIT_FIELD_REF with > vector type retain the original mode. > > * gcc.target/i386/pr91897.c: New testcase. This looks good to me, thanks. -- Eric Botcazou
Re: [PATCH] Retain TYPE_MODE more often for BIT_FIELD_REFs in get_inner_referece
On Thu, 26 Sep 2019, Eric Botcazou wrote: > > I see. So I misremember seeing aggregate typed BIT_FIELD_REFs > > (that was probably VIEW_CONVERTs then...). Still the GIMPLE verifier > > only has > > > > else if (!INTEGRAL_TYPE_P (TREE_TYPE (expr)) > >&& TYPE_MODE (TREE_TYPE (expr)) != BLKmode > >&& maybe_ne (GET_MODE_BITSIZE (TYPE_MODE (TREE_TYPE > > (expr))), > > size)) > > { > > error ("mode size of non-integral result does not " > > "match field size of %qs", > > code_name); > > return true; > > } > > > > it doesn't verify that besides integral typed expressions only > > vector typed expressions are allowed. > > I think that the !INTEGRAL_TYPE_P is simply the opposite of the first case: > > if (INTEGRAL_TYPE_P (TREE_TYPE (t)) > && maybe_ne (TYPE_PRECISION (TREE_TYPE (t)), size)) > { > error ("integral result type precision does not match " >"field size of BIT_FIELD_REF"); > return t; > } > > > Anyhow - the original patch succeeded bootstrapping and testing. > > The way I proposed it: > > > > /* For vector types, with the correct size of access, use the mode > > of > > inner type. */ > > if (((TREE_CODE (TREE_TYPE (TREE_OPERAND (exp, 0))) == VECTOR_TYPE > > && TREE_TYPE (exp) == TREE_TYPE (TREE_TYPE (TREE_OPERAND (exp, > > 0 > > > >|| !INTEGRAL_TYPE_P (TREE_TYPE (exp))) > > > > && tree_int_cst_equal (size_tree, TYPE_SIZE (TREE_TYPE (exp > > mode = TYPE_MODE (TREE_TYPE (exp)); > > > > matches in sofar that we only restrict integer types (not modes) and > > for integer types allow extracts from vectors (the preexisting > > check for a matching component type is a bit too strict I guess). > > IMO if the !VECTOR_TYPE case cannot be covered, changes in this delicate area > ought to be restricted to VECTOR_TYPE. OK, so I'm testing the following then, simply adding the VECTOR_TYPE_P case in addition to the existing one plus adjusting the comment accordingly. Bootstrap / regtest running on x86_64-unknown-linux-gnu, OK if that passes? Thanks, Richard. 2019-09-26 Richard Biener PR middle-end/91897 * expr.c (get_inner_reference): For BIT_FIELD_REF with vector type retain the original mode. * gcc.target/i386/pr91897.c: New testcase. Index: gcc/testsuite/gcc.target/i386/pr91897.c === --- gcc/testsuite/gcc.target/i386/pr91897.c (nonexistent) +++ gcc/testsuite/gcc.target/i386/pr91897.c (working copy) @@ -0,0 +1,12 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -mavx" } */ + +typedef double Double16 __attribute__((vector_size(8*16))); + +void mult(Double16 *res, const Double16 *v1, const Double16 *v2) +{ + *res = *v1 * *v2; +} + +/* We want 4 ymm loads and 4 ymm stores. */ +/* { dg-final { scan-assembler-times "movapd" 8 } } */ Index: gcc/expr.c === --- gcc/expr.c (revision 276147) +++ gcc/expr.c (working copy) @@ -7230,12 +7230,13 @@ get_inner_reference (tree exp, poly_int6 *punsignedp = (! INTEGRAL_TYPE_P (TREE_TYPE (exp)) || TYPE_UNSIGNED (TREE_TYPE (exp))); - /* For vector types, with the correct size of access, use the mode of -inner type. */ - if (TREE_CODE (TREE_TYPE (TREE_OPERAND (exp, 0))) == VECTOR_TYPE - && TREE_TYPE (exp) == TREE_TYPE (TREE_TYPE (TREE_OPERAND (exp, 0))) - && tree_int_cst_equal (size_tree, TYPE_SIZE (TREE_TYPE (exp -mode = TYPE_MODE (TREE_TYPE (exp)); + /* For vector element types with the correct size of access or for + vector typed accesses use the mode of the access type. */ + if ((TREE_CODE (TREE_TYPE (TREE_OPERAND (exp, 0))) == VECTOR_TYPE + && TREE_TYPE (exp) == TREE_TYPE (TREE_TYPE (TREE_OPERAND (exp, 0))) + && tree_int_cst_equal (size_tree, TYPE_SIZE (TREE_TYPE (exp + || VECTOR_TYPE_P (TREE_TYPE (exp))) + mode = TYPE_MODE (TREE_TYPE (exp)); } else {
Re: [PATCH] Retain TYPE_MODE more often for BIT_FIELD_REFs in get_inner_referece
> I see. So I misremember seeing aggregate typed BIT_FIELD_REFs > (that was probably VIEW_CONVERTs then...). Still the GIMPLE verifier > only has > > else if (!INTEGRAL_TYPE_P (TREE_TYPE (expr)) >&& TYPE_MODE (TREE_TYPE (expr)) != BLKmode >&& maybe_ne (GET_MODE_BITSIZE (TYPE_MODE (TREE_TYPE > (expr))), > size)) > { > error ("mode size of non-integral result does not " > "match field size of %qs", > code_name); > return true; > } > > it doesn't verify that besides integral typed expressions only > vector typed expressions are allowed. I think that the !INTEGRAL_TYPE_P is simply the opposite of the first case: if (INTEGRAL_TYPE_P (TREE_TYPE (t)) && maybe_ne (TYPE_PRECISION (TREE_TYPE (t)), size)) { error ("integral result type precision does not match " "field size of BIT_FIELD_REF"); return t; } > Anyhow - the original patch succeeded bootstrapping and testing. > The way I proposed it: > > /* For vector types, with the correct size of access, use the mode > of > inner type. */ > if (((TREE_CODE (TREE_TYPE (TREE_OPERAND (exp, 0))) == VECTOR_TYPE > && TREE_TYPE (exp) == TREE_TYPE (TREE_TYPE (TREE_OPERAND (exp, > 0 > >|| !INTEGRAL_TYPE_P (TREE_TYPE (exp))) > > && tree_int_cst_equal (size_tree, TYPE_SIZE (TREE_TYPE (exp > mode = TYPE_MODE (TREE_TYPE (exp)); > > matches in sofar that we only restrict integer types (not modes) and > for integer types allow extracts from vectors (the preexisting > check for a matching component type is a bit too strict I guess). IMO if the !VECTOR_TYPE case cannot be covered, changes in this delicate area ought to be restricted to VECTOR_TYPE. -- Eric Botcazou
Re: [PATCH] Retain TYPE_MODE more often for BIT_FIELD_REFs in get_inner_referece
On Wed, 25 Sep 2019, Eric Botcazou wrote: > > For the PR it would be good enough. Though I wonder what the original reason > > for the mode handling was. Was it to avoid not naturally aligned modes for > > strict align targets? Or modes for non-mode size entities? > > Bit-field extraction ultimately required integer modes before vector modes > came to light so I think that preserving their original mode was useless. I see. So I misremember seeing aggregate typed BIT_FIELD_REFs (that was probably VIEW_CONVERTs then...). Still the GIMPLE verifier only has else if (!INTEGRAL_TYPE_P (TREE_TYPE (expr)) && TYPE_MODE (TREE_TYPE (expr)) != BLKmode && maybe_ne (GET_MODE_BITSIZE (TYPE_MODE (TREE_TYPE (expr))), size)) { error ("mode size of non-integral result does not " "match field size of %qs", code_name); return true; } it doesn't verify that besides integral typed expressions only vector typed expressions are allowed. Anyhow - the original patch succeeded bootstrapping and testing. The way I proposed it: /* For vector types, with the correct size of access, use the mode of inner type. */ if (((TREE_CODE (TREE_TYPE (TREE_OPERAND (exp, 0))) == VECTOR_TYPE && TREE_TYPE (exp) == TREE_TYPE (TREE_TYPE (TREE_OPERAND (exp, 0 || !INTEGRAL_TYPE_P (TREE_TYPE (exp))) && tree_int_cst_equal (size_tree, TYPE_SIZE (TREE_TYPE (exp mode = TYPE_MODE (TREE_TYPE (exp)); matches in sofar that we only restrict integer types (not modes) and for integer types allow extracts from vectors (the preexisting check for a matching component type is a bit too strict I guess). Thus, OK with adjusting the comment to reflect the change? Thanks, Richard.
Re: [PATCH] Retain TYPE_MODE more often for BIT_FIELD_REFs in get_inner_referece
> For the PR it would be good enough. Though I wonder what the original reason > for the mode handling was. Was it to avoid not naturally aligned modes for > strict align targets? Or modes for non-mode size entities? Bit-field extraction ultimately required integer modes before vector modes came to light so I think that preserving their original mode was useless. -- Eric Botcazou
Re: [PATCH] Retain TYPE_MODE more often for BIT_FIELD_REFs in get_inner_referece
On September 25, 2019 5:29:55 PM GMT+02:00, Eric Botcazou wrote: >> The following patch relaxes the condition under which we force >> VOIDmode by making all non-integral types where the extraction >> size matches the type size (thus isn't "bitfieldish") use the >> mode of the extraction type. > >Wouldn't TREE_CODE (TREE_TYPE (exp)) == VECTOR_TYPE be sufficient? At >least >this would still be in keeping with the comment... For the PR it would be good enough. Though I wonder what the original reason for the mode handling was. Was it to avoid not naturally aligned modes for strict align targets? Or modes for non-mode size entities? Richard.
Re: [PATCH] Retain TYPE_MODE more often for BIT_FIELD_REFs in get_inner_referece
> The following patch relaxes the condition under which we force > VOIDmode by making all non-integral types where the extraction > size matches the type size (thus isn't "bitfieldish") use the > mode of the extraction type. Wouldn't TREE_CODE (TREE_TYPE (exp)) == VECTOR_TYPE be sufficient? At least this would still be in keeping with the comment... -- Eric Botcazou
[PATCH] Retain TYPE_MODE more often for BIT_FIELD_REFs in get_inner_referece
BIT_FIELD_REFs can extract almost any kind of type but specifically is used to extract vector elements (that very special case is handled) but also sub-vectors (missing). RTL expansion of stores relies on an appropriate mode to use vector stores it seems. The following patch relaxes the condition under which we force VOIDmode by making all non-integral types where the extraction size matches the type size (thus isn't "bitfieldish") use the mode of the extraction type. The patch leaves alone things like QImode extracts from SImode since that would need to check the offset as well whereas I assume we cannot extract non-INTEGRAL entities at non-byte aligned offsets(?). Bootstrap / regtest running on x86_64-unknown-linux-gnu. OK for trunk? Thanks, Richard. 2019-09-25 Richard Biener PR middle-end/91897 * expr.c (get_inner_reference): For BIT_FIELD_REF with non-integral type and matching access size retain the original mode. * gcc.target/i386/pr91897.c: New testcase. Index: gcc/expr.c === --- gcc/expr.c (revision 276123) +++ gcc/expr.c (working copy) @@ -7232,8 +7232,9 @@ get_inner_reference (tree exp, poly_int6 /* For vector types, with the correct size of access, use the mode of inner type. */ - if (TREE_CODE (TREE_TYPE (TREE_OPERAND (exp, 0))) == VECTOR_TYPE - && TREE_TYPE (exp) == TREE_TYPE (TREE_TYPE (TREE_OPERAND (exp, 0))) + if (((TREE_CODE (TREE_TYPE (TREE_OPERAND (exp, 0))) == VECTOR_TYPE + && TREE_TYPE (exp) == TREE_TYPE (TREE_TYPE (TREE_OPERAND (exp, 0 + || !INTEGRAL_TYPE_P (TREE_TYPE (exp))) && tree_int_cst_equal (size_tree, TYPE_SIZE (TREE_TYPE (exp mode = TYPE_MODE (TREE_TYPE (exp)); } Index: gcc/testsuite/gcc.target/i386/pr91897.c === --- gcc/testsuite/gcc.target/i386/pr91897.c (nonexistent) +++ gcc/testsuite/gcc.target/i386/pr91897.c (working copy) @@ -0,0 +1,12 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -mavx" } */ + +typedef double Double16 __attribute__((vector_size(8*16))); + +void mult(Double16 *res, const Double16 *v1, const Double16 *v2) +{ + *res = *v1 * *v2; +} + +/* We want 4 ymm loads and 4 ymm stores. */ +/* { dg-final { scan-assembler-times "movapd" 8 } } */