Re: [PATCH] Fix expansion of TREE_ADDRESSABLE bitwise copies (PR c++/69851)

Re: [PATCH] Fix expansion of TREE_ADDRESSABLE bitwise copies (PR c++/69851)

[Bernd Edlinger]

Hi,


> --- gcc/expr.c.jj	2016-02-12 00:50:55.000000000 +0100
> +++ gcc/expr.c	2016-02-19 10:43:59.639162531 +0100
> @@ -6643,14 +6643,24 @@ store_field (rtx target, HOST_WIDE_INT b
>  	  /* Except for initialization of full bytes from a CONSTRUCTOR, which
>  	     we will handle specially below.  */
>  	  && !(TREE_CODE (exp) == CONSTRUCTOR
> -	       && bitsize % BITS_PER_UNIT == 0))
> +	       && bitsize % BITS_PER_UNIT == 0)
> +	  /* And except for bitwise copying of TREE_ADDRESSABLE types,
> +	     where the FIELD_DECL has the right bitsize, but TREE_TYPE (exp)
> +	     includes some extra padding.  */
> +	  && (!TREE_ADDRESSABLE (TREE_TYPE (exp))
> +	      || TREE_CODE (exp) != COMPONENT_REF
> +	      || TREE_CODE (DECL_SIZE (TREE_OPERAND (exp, 1))) != INTEGER_CST
> +	      || (bitsize % BITS_PER_UNIT != 0)
> +	      || (bitpos % BITS_PER_UNIT != 0)
> +	      || (compare_tree_int (DECL_SIZE (TREE_OPERAND (exp, 1)), bitsize)
> +		  != 0)))
>        /* If we are expanding a MEM_REF of a non-BLKmode non-addressable
>           decl we must use bitfield operations.  */

isn't the indentation of the new block wrong?


Bernd.

Re: [PATCH] Fix expansion of TREE_ADDRESSABLE bitwise copies (PR c++/69851)

[Jakub Jelinek]

On Fri, Feb 19, 2016 at 06:42:32PM +0000, Bernd Edlinger wrote:
> Hi,
> 
> 
> > --- gcc/expr.c.jj	2016-02-12 00:50:55.000000000 +0100
> > +++ gcc/expr.c	2016-02-19 10:43:59.639162531 +0100
> > @@ -6643,14 +6643,24 @@ store_field (rtx target, HOST_WIDE_INT b
> >  	  /* Except for initialization of full bytes from a CONSTRUCTOR, which
> >  	     we will handle specially below.  */
> >  	  && !(TREE_CODE (exp) == CONSTRUCTOR
> > -	       && bitsize % BITS_PER_UNIT == 0))
> > +	       && bitsize % BITS_PER_UNIT == 0)
> > +	  /* And except for bitwise copying of TREE_ADDRESSABLE types,
> > +	     where the FIELD_DECL has the right bitsize, but TREE_TYPE (exp)
> > +	     includes some extra padding.  */
> > +	  && (!TREE_ADDRESSABLE (TREE_TYPE (exp))
> > +	      || TREE_CODE (exp) != COMPONENT_REF
> > +	      || TREE_CODE (DECL_SIZE (TREE_OPERAND (exp, 1))) != INTEGER_CST
> > +	      || (bitsize % BITS_PER_UNIT != 0)
> > +	      || (bitpos % BITS_PER_UNIT != 0)
> > +	      || (compare_tree_int (DECL_SIZE (TREE_OPERAND (exp, 1)), bitsize)
> > +		  != 0)))
> >        /* If we are expanding a MEM_REF of a non-BLKmode non-addressable
> >           decl we must use bitfield operations.  */
> 
> isn't the indentation of the new block wrong?

No, I think it is correct.

	Jakub

Re: [PATCH] Fix expansion of TREE_ADDRESSABLE bitwise copies (PR c++/69851)

[Bernd Edlinger]

On 19.02.2016 20:04, Jakub Jelinek wrote:
> On Fri, Feb 19, 2016 at 06:42:32PM +0000, Bernd Edlinger wrote:
>> Hi,
>>
>>
>>> --- gcc/expr.c.jj	2016-02-12 00:50:55.000000000 +0100
>>> +++ gcc/expr.c	2016-02-19 10:43:59.639162531 +0100
>>> @@ -6643,14 +6643,24 @@ store_field (rtx target, HOST_WIDE_INT b
>>>   	  /* Except for initialization of full bytes from a CONSTRUCTOR, which
>>>   	     we will handle specially below.  */
>>>   	  && !(TREE_CODE (exp) == CONSTRUCTOR
>>> -	       && bitsize % BITS_PER_UNIT == 0))
>>> +	       && bitsize % BITS_PER_UNIT == 0)
>>> +	  /* And except for bitwise copying of TREE_ADDRESSABLE types,
>>> +	     where the FIELD_DECL has the right bitsize, but TREE_TYPE (exp)
>>> +	     includes some extra padding.  */
>>> +	  && (!TREE_ADDRESSABLE (TREE_TYPE (exp))
>>> +	      || TREE_CODE (exp) != COMPONENT_REF
>>> +	      || TREE_CODE (DECL_SIZE (TREE_OPERAND (exp, 1))) != INTEGER_CST
>>> +	      || (bitsize % BITS_PER_UNIT != 0)
>>> +	      || (bitpos % BITS_PER_UNIT != 0)
>>> +	      || (compare_tree_int (DECL_SIZE (TREE_OPERAND (exp, 1)), bitsize)
>>> +		  != 0)))
>>>         /* If we are expanding a MEM_REF of a non-BLKmode non-addressable
>>>            decl we must use bitfield operations.  */
>>
>> isn't the indentation of the new block wrong?
>
> No, I think it is correct.
>
> 	Jakub
>


but you are just adding another term to this expression:
  !(TREE_CODE (exp) == CONSTRUCTOR
    && bitsize % BITS_PER_UNIT == 0)

so the result should look like
  !(TREE_CODE (exp) == CONSTRUCTOR
   && bitsize % BITS_PER_UNIT == 0
   && (!TREE_ADDRESSABLE ...
       || TREE_CODE () ...
       ...
       || (compare_tree_int ...
           != 0)))



Bernd.

Re: [PATCH] Fix expansion of TREE_ADDRESSABLE bitwise copies (PR c++/69851)

[Jakub Jelinek]

On Fri, Feb 19, 2016 at 08:04:39PM +0000, Bernd Edlinger wrote:
> but you are just adding another term to this expression:
>   !(TREE_CODE (exp) == CONSTRUCTOR
>     && bitsize % BITS_PER_UNIT == 0)

No.  Please read the code again.  I'm adding another case
after this one.

> so the result should look like
>   !(TREE_CODE (exp) == CONSTRUCTOR
>    && bitsize % BITS_PER_UNIT == 0
>    && (!TREE_ADDRESSABLE ...
>        || TREE_CODE () ...
>        ...
>        || (compare_tree_int ...
>            != 0)))

	Jakub

Re: [PATCH] Fix expansion of TREE_ADDRESSABLE bitwise copies (PR c++/69851)

[Bernd Edlinger]

On 19.02.2016 21:08, Jakub Jelinek wrote:
> On Fri, Feb 19, 2016 at 08:04:39PM +0000, Bernd Edlinger wrote:
>> but you are just adding another term to this expression:
>>    !(TREE_CODE (exp) == CONSTRUCTOR
>>      && bitsize % BITS_PER_UNIT == 0)
>
> No.  Please read the code again.  I'm adding another case
> after this one.
>

Ok, now I see.
Sorry for the noise.


Bernd.

Re: [PATCH] Fix expansion of TREE_ADDRESSABLE bitwise copies (PR c++/69851)

[Bernd Edlinger]

Excuse me,

I have still another question.

Why are you adding braces here?

+	      || (bitsize % BITS_PER_UNIT != 0)
+	      || (bitpos % BITS_PER_UNIT != 0)
+	      || (compare_tree_int (DECL_SIZE (TREE_OPERAND (exp, 1)), bitsize)
+		  != 0)))

I think everywhere in that function we omit braces around == terms
inside || terms even long ones.

    || a == b
    || c == d
    || e == f)


I know many like to add braces here, but GCC does not do that
unnecessarily, right?


Thanks
Bernd.

Re: [PATCH] Fix expansion of TREE_ADDRESSABLE bitwise copies (PR c++/69851)

[Jakub Jelinek]

On Fri, Feb 19, 2016 at 08:45:09PM +0000, Bernd Edlinger wrote:
> I have still another question.
> 
> Why are you adding braces here?
> 
> +	      || (bitsize % BITS_PER_UNIT != 0)
> +	      || (bitpos % BITS_PER_UNIT != 0)

These two are not really needed, but I've already committed the patch.

> +	      || (compare_tree_int (DECL_SIZE (TREE_OPERAND (exp, 1)), bitsize)
> +		  != 0)))
> 
> I think everywhere in that function we omit braces around == terms
> inside || terms even long ones.

emacs users keep saying that emacs needs this to indent it properly.

>     || a == b
>     || c == d
>     || e == f)

These are on a single line, so it is not a problem.

	Jakub

Re: [PATCH] Fix expansion of TREE_ADDRESSABLE bitwise copies (PR c++/69851)

[Bernd Edlinger]

Excuse me,

I have still another question.

Why are you adding braces here?

+	      || (bitsize % BITS_PER_UNIT != 0)
+	      || (bitpos % BITS_PER_UNIT != 0)
+	      || (compare_tree_int (DECL_SIZE (TREE_OPERAND (exp, 1)), bitsize)
+		  != 0)))

I think everywhere in that function we omit braces around == terms
inside || terms even long ones.

    || a == b
    || c == d
    || e == f)


I know many like to add braces here, but GCC does not do that
unnecessarily, right?


Thanks
Bernd.

[PATCH] Fix expansion of TREE_ADDRESSABLE bitwise copies (PR c++/69851)

[Jakub Jelinek]

Hi!

As described in the PR, in C++ we can have assignments
where both the lhs and rhs are COMPONENT_REFs with TREE_ADDRESSABLE types,
including padding, but the FIELD_DECLs are artificial fields that have
narrower bit sizes.
store_field in this case takes the path of bit-field handling (even when
it has bitpos and bitsize multiples of BITS_PER_UNIT (I think that is
necessarily true for the TREE_ADDRESSABLE types), which is incorrect,
because the rhs is expanded in that case through expand_normal, which
for a result type wider than the FIELD_DECL with forces it into a temporary.
In older GCCs that just generated inefficient code (copy the rhs into a
stack temporary, then copy that to lhs), but GCC trunk ICEs on that.
Fixed by not taking the bit-field path in that case after verifying
we'll be able to expand it properly using the normal store_expr.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2016-02-19  Jakub Jelinek  <jakub@redhat.com>

	PR c++/69851
	* expr.c (store_field): Don't use bit-field path if exp is
	COMPONENT_REF with TREE_ADDRESSABLE type, where TYPE_SIZE is
	different from bitsize, but DECL_SIZE of FIELD_DECL is bitsize
	and the assignment can be performed by bitwise copy.  Formatting
	fix.

	* g++.dg/torture/pr69851.C: New test.

--- gcc/expr.c.jj	2016-02-12 00:50:55.000000000 +0100
+++ gcc/expr.c	2016-02-19 10:43:59.639162531 +0100
@@ -6643,14 +6643,24 @@ store_field (rtx target, HOST_WIDE_INT b
 	  /* Except for initialization of full bytes from a CONSTRUCTOR, which
 	     we will handle specially below.  */
 	  && !(TREE_CODE (exp) == CONSTRUCTOR
-	       && bitsize % BITS_PER_UNIT == 0))
+	       && bitsize % BITS_PER_UNIT == 0)
+	  /* And except for bitwise copying of TREE_ADDRESSABLE types,
+	     where the FIELD_DECL has the right bitsize, but TREE_TYPE (exp)
+	     includes some extra padding.  */
+	  && (!TREE_ADDRESSABLE (TREE_TYPE (exp))
+	      || TREE_CODE (exp) != COMPONENT_REF
+	      || TREE_CODE (DECL_SIZE (TREE_OPERAND (exp, 1))) != INTEGER_CST
+	      || (bitsize % BITS_PER_UNIT != 0)
+	      || (bitpos % BITS_PER_UNIT != 0)
+	      || (compare_tree_int (DECL_SIZE (TREE_OPERAND (exp, 1)), bitsize)
+		  != 0)))
       /* If we are expanding a MEM_REF of a non-BLKmode non-addressable
          decl we must use bitfield operations.  */
       || (bitsize >= 0
 	  && TREE_CODE (exp) == MEM_REF
 	  && TREE_CODE (TREE_OPERAND (exp, 0)) == ADDR_EXPR
 	  && DECL_P (TREE_OPERAND (TREE_OPERAND (exp, 0), 0))
-	  && !TREE_ADDRESSABLE (TREE_OPERAND (TREE_OPERAND (exp, 0),0 ))
+	  && !TREE_ADDRESSABLE (TREE_OPERAND (TREE_OPERAND (exp, 0), 0))
 	  && DECL_MODE (TREE_OPERAND (TREE_OPERAND (exp, 0), 0)) != BLKmode))
     {
       rtx temp;
--- gcc/testsuite/g++.dg/torture/pr69851.C.jj	2016-02-19 10:59:22.224438830 +0100
+++ gcc/testsuite/g++.dg/torture/pr69851.C	2016-02-19 10:59:12.000000000 +0100
@@ -0,0 +1,24 @@
+// PR c++/69851
+// { dg-do compile }
+// { dg-options "-std=c++11" }
+
+template <typename T>
+struct A { T a; };
+template <unsigned long, typename...>
+struct B;
+template <unsigned long N, typename T, typename... U>
+struct B<N, T, U...> : B<1, U...>, A<T>
+{
+  B (B &) = default;
+  B (B &&x) : B(x) {}
+};
+template <unsigned long N, typename T>
+struct B<N, T> {};
+struct C { C (C &); };
+struct D {};
+
+void
+foo (B<0, C, D, int, int> a)
+{
+  B<0, C, D, int, int> b (a);
+}

	Jakub

Re: [PATCH] Fix expansion of TREE_ADDRESSABLE bitwise copies (PR c++/69851)

[Jason Merrill]

On 02/19/2016 09:03 AM, Jakub Jelinek wrote:
> As described in the PR, in C++ we can have assignments
> where both the lhs and rhs are COMPONENT_REFs with TREE_ADDRESSABLE types,
> including padding, but the FIELD_DECLs are artificial fields that have
> narrower bit sizes.
> store_field in this case takes the path of bit-field handling (even when
> it has bitpos and bitsize multiples of BITS_PER_UNIT (I think that is
> necessarily true for the TREE_ADDRESSABLE types), which is incorrect,
> because the rhs is expanded in that case through expand_normal, which
> for a result type wider than the FIELD_DECL with forces it into a temporary.
> In older GCCs that just generated inefficient code (copy the rhs into a
> stack temporary, then copy that to lhs), but GCC trunk ICEs on that.
> Fixed by not taking the bit-field path in that case after verifying
> we'll be able to expand it properly using the normal store_expr.

Won't store_expr clobber tail padding because it doesn't know about 
bitsize?  I would think that what we want is to use emit_block_move in 
the bit-field path whenever we're dealing with whole bytes in memory.

Jason

Re: [PATCH] Fix expansion of TREE_ADDRESSABLE bitwise copies (PR c++/69851)

[Jakub Jelinek]

On Fri, Feb 19, 2016 at 01:30:52PM -0500, Jason Merrill wrote:
> On 02/19/2016 09:03 AM, Jakub Jelinek wrote:
> >As described in the PR, in C++ we can have assignments
> >where both the lhs and rhs are COMPONENT_REFs with TREE_ADDRESSABLE types,
> >including padding, but the FIELD_DECLs are artificial fields that have
> >narrower bit sizes.
> >store_field in this case takes the path of bit-field handling (even when
> >it has bitpos and bitsize multiples of BITS_PER_UNIT (I think that is
> >necessarily true for the TREE_ADDRESSABLE types), which is incorrect,
> >because the rhs is expanded in that case through expand_normal, which
> >for a result type wider than the FIELD_DECL with forces it into a temporary.
> >In older GCCs that just generated inefficient code (copy the rhs into a
> >stack temporary, then copy that to lhs), but GCC trunk ICEs on that.
> >Fixed by not taking the bit-field path in that case after verifying
> >we'll be able to expand it properly using the normal store_expr.
> 
> Won't store_expr clobber tail padding because it doesn't know about bitsize?

It doesn't clobber it, because it uses get_inner_reference, expands the
inner reference (which is necessarily for something TREE_ADDRESSABLE either
a MEM_REF or some decl that lives in memory), and get_inner_reference in
that case gives it the bitsize/bitpos from the FIELD_DECL.
Which is why in the patch I've posted there is the comparison of DECL_SIZE
of the FIELD_DECL against the bitsize that is passed to store_field.

> I would think that what we want is to use emit_block_move in the bit-field
> path whenever we're dealing with whole bytes in memory.

I'm afraid we'd need to duplicate too much code if we'd wanted to bypass all
the layers in there.

	Jakub

Re: [PATCH] Fix expansion of TREE_ADDRESSABLE bitwise copies (PR c++/69851)

[Jason Merrill]

On 02/19/2016 01:41 PM, Jakub Jelinek wrote:
> On Fri, Feb 19, 2016 at 01:30:52PM -0500, Jason Merrill wrote:
>> On 02/19/2016 09:03 AM, Jakub Jelinek wrote:
>>> As described in the PR, in C++ we can have assignments
>>> where both the lhs and rhs are COMPONENT_REFs with TREE_ADDRESSABLE types,
>>> including padding, but the FIELD_DECLs are artificial fields that have
>>> narrower bit sizes.
>>> store_field in this case takes the path of bit-field handling (even when
>>> it has bitpos and bitsize multiples of BITS_PER_UNIT (I think that is
>>> necessarily true for the TREE_ADDRESSABLE types), which is incorrect,
>>> because the rhs is expanded in that case through expand_normal, which
>>> for a result type wider than the FIELD_DECL with forces it into a temporary.
>>> In older GCCs that just generated inefficient code (copy the rhs into a
>>> stack temporary, then copy that to lhs), but GCC trunk ICEs on that.
>>> Fixed by not taking the bit-field path in that case after verifying
>>> we'll be able to expand it properly using the normal store_expr.
>>
>> Won't store_expr clobber tail padding because it doesn't know about bitsize?
>
> It doesn't clobber it, because it uses get_inner_reference, expands the
> inner reference (which is necessarily for something TREE_ADDRESSABLE either
> a MEM_REF or some decl that lives in memory), and get_inner_reference in
> that case gives it the bitsize/bitpos from the FIELD_DECL.
> Which is why in the patch I've posted there is the comparison of DECL_SIZE
> of the FIELD_DECL against the bitsize that is passed to store_field.

Ah, that makes sense.  Please mention that in your added comment.

For GCC 7, can we drop the TREE_ADDRESSABLE check?

Jason

Re: [PATCH] Fix expansion of TREE_ADDRESSABLE bitwise copies (PR c++/69851)

[Jakub Jelinek]

On Fri, Feb 19, 2016 at 02:00:07PM -0500, Jason Merrill wrote:
> On 02/19/2016 01:41 PM, Jakub Jelinek wrote:
> >On Fri, Feb 19, 2016 at 01:30:52PM -0500, Jason Merrill wrote:
> >>On 02/19/2016 09:03 AM, Jakub Jelinek wrote:
> >>>As described in the PR, in C++ we can have assignments
> >>>where both the lhs and rhs are COMPONENT_REFs with TREE_ADDRESSABLE types,
> >>>including padding, but the FIELD_DECLs are artificial fields that have
> >>>narrower bit sizes.
> >>>store_field in this case takes the path of bit-field handling (even when
> >>>it has bitpos and bitsize multiples of BITS_PER_UNIT (I think that is
> >>>necessarily true for the TREE_ADDRESSABLE types), which is incorrect,
> >>>because the rhs is expanded in that case through expand_normal, which
> >>>for a result type wider than the FIELD_DECL with forces it into a temporary.
> >>>In older GCCs that just generated inefficient code (copy the rhs into a
> >>>stack temporary, then copy that to lhs), but GCC trunk ICEs on that.
> >>>Fixed by not taking the bit-field path in that case after verifying
> >>>we'll be able to expand it properly using the normal store_expr.
> >>
> >>Won't store_expr clobber tail padding because it doesn't know about bitsize?
> >
> >It doesn't clobber it, because it uses get_inner_reference, expands the
> >inner reference (which is necessarily for something TREE_ADDRESSABLE either
> >a MEM_REF or some decl that lives in memory), and get_inner_reference in
> >that case gives it the bitsize/bitpos from the FIELD_DECL.
> >Which is why in the patch I've posted there is the comparison of DECL_SIZE
> >of the FIELD_DECL against the bitsize that is passed to store_field.
> 
> Ah, that makes sense.  Please mention that in your added comment.
> 
> For GCC 7, can we drop the TREE_ADDRESSABLE check?

I think we can't drop it, but we could replace it with a check that
get_inner_reference is something that must live in memory
(MEM_REF/TARGET_MEM_REF of SSA_NAME, or of decl that lives in memory,
or decl itself that lives in memory).

	Jakub

Re: [PATCH] Fix expansion of TREE_ADDRESSABLE bitwise copies (PR c++/69851)

[Jason Merrill]

On 02/19/2016 02:07 PM, Jakub Jelinek wrote:
> On Fri, Feb 19, 2016 at 02:00:07PM -0500, Jason Merrill wrote:
>> On 02/19/2016 01:41 PM, Jakub Jelinek wrote:
>>> On Fri, Feb 19, 2016 at 01:30:52PM -0500, Jason Merrill wrote:
>>>> On 02/19/2016 09:03 AM, Jakub Jelinek wrote:
>>>>> As described in the PR, in C++ we can have assignments
>>>>> where both the lhs and rhs are COMPONENT_REFs with TREE_ADDRESSABLE types,
>>>>> including padding, but the FIELD_DECLs are artificial fields that have
>>>>> narrower bit sizes.
>>>>> store_field in this case takes the path of bit-field handling (even when
>>>>> it has bitpos and bitsize multiples of BITS_PER_UNIT (I think that is
>>>>> necessarily true for the TREE_ADDRESSABLE types), which is incorrect,
>>>>> because the rhs is expanded in that case through expand_normal, which
>>>>> for a result type wider than the FIELD_DECL with forces it into a temporary.
>>>>> In older GCCs that just generated inefficient code (copy the rhs into a
>>>>> stack temporary, then copy that to lhs), but GCC trunk ICEs on that.
>>>>> Fixed by not taking the bit-field path in that case after verifying
>>>>> we'll be able to expand it properly using the normal store_expr.
>>>>
>>>> Won't store_expr clobber tail padding because it doesn't know about bitsize?
>>>
>>> It doesn't clobber it, because it uses get_inner_reference, expands the
>>> inner reference (which is necessarily for something TREE_ADDRESSABLE either
>>> a MEM_REF or some decl that lives in memory), and get_inner_reference in
>>> that case gives it the bitsize/bitpos from the FIELD_DECL.
>>> Which is why in the patch I've posted there is the comparison of DECL_SIZE
>>> of the FIELD_DECL against the bitsize that is passed to store_field.
>>
>> Ah, that makes sense.  Please mention that in your added comment.
>>
>> For GCC 7, can we drop the TREE_ADDRESSABLE check?
>
> I think we can't drop it, but we could replace it with a check that
> get_inner_reference is something that must live in memory
> (MEM_REF/TARGET_MEM_REF of SSA_NAME, or of decl that lives in memory,
> or decl itself that lives in memory).

Please mention that in the comment, as well.  OK with those comment changes.

Jason