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  <rguenther@suse.de>

	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.

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

On September 25, 2019 5:29:55 PM GMT+02:00, Eric Botcazou <ebotcazou@adacore.com> 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.

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

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.

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

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  <rguenther@suse.de>

	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
     {

> 2019-09-26  Richard Biener  <rguenther@suse.de>
> 
> 	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.