Re: [PATCH][PR c++/82888] smarter code for default initialization of scalar arrays

Re: [PATCH][PR c++/82888] smarter code for default initialization of scalar arrays

[Jason Merrill]

On Thu, Nov 16, 2017 at 11:21 AM, Nathan Froyd <froydnj@mozilla.com> wrote:
> Default-initialization of scalar arrays in C++ member initialization
> lists produced rather slow code, laboriously setting each element of the
> array to zero.  It would be much faster to block-initialize the array,
> and that's what this patch does.
>
> The patch works for me, but I'm not sure if it's the best way to
> accomplish this.  At least two other possibilities come to mind:
>
> 1) Detect this case in build_vec_init_expr and act as though the user
>    wrote 'member{0}', which the front-end already produces efficient
>    code for.
>
> 2) Detect this case in build_vec_init, but again, act as though the user
>    wrote 'member{0}' and let everything proceed as normal.
>    (Alternatively, handle this case prior to calling build_vec_init and
>    pass different arguments to build_vec_init.)
>
> Opinions as to the best way forward here?  I'm unsure of whether the
> code below is front-end friendly; I see in the gimple dumps that the
> solution below adds an extra CLOBBER on 'this' for 'member()', whereas
> 'member{0}' does not.  It's possible that I'm missing something.
>
> Bootstrapped on x86_64-unknown-linux-gnu, no regressions.
>
> OK for trunk?
>
> -Nathan
>
> gcc/cp/
>         PR c++/82888
>         * init.c (build_vec_init): Handle default-initialization of array
>         types.
>
> gcc/testsuite/
>         PR c++/82888
>         * g++.dg/init/pr82888.C: New.
>
> diff --git a/gcc/cp/init.c b/gcc/cp/init.c
> index c76460d..53d6133 100644
> --- a/gcc/cp/init.c
> +++ b/gcc/cp/init.c
> @@ -4038,6 +4038,15 @@ build_vec_init (tree base, tree maxindex, tree init,
>         }
>      }
>
> +  /* Default-initialize scalar arrays directly.  */
> +  if (TREE_CODE (atype) == ARRAY_TYPE
> +      && SCALAR_TYPE_P (TREE_TYPE (atype))
> +      && !init)

This should check explicit_value_init._p rather than !init.  And also
check zero_init_p.

> +    {
> +      gcc_assert (!from_array);
> +      return build2 (MODIFY_EXPR, atype, base, build_constructor (atype, NULL));

And this should use INIT_EXPR.

Jason

Re: [PATCH][PR c++/82888] smarter code for default initialization of scalar arrays

[Nathan Froyd]

On Fri, Nov 17, 2017 at 8:50 AM, Jason Merrill <jason@redhat.com> wrote:
> On Thu, Nov 16, 2017 at 11:21 AM, Nathan Froyd <froydnj@mozilla.com> wrote:
>> diff --git a/gcc/cp/init.c b/gcc/cp/init.c
>> index c76460d..53d6133 100644
>> --- a/gcc/cp/init.c
>> +++ b/gcc/cp/init.c
>> @@ -4038,6 +4038,15 @@ build_vec_init (tree base, tree maxindex, tree init,
>>         }
>>      }
>>
>> +  /* Default-initialize scalar arrays directly.  */
>> +  if (TREE_CODE (atype) == ARRAY_TYPE
>> +      && SCALAR_TYPE_P (TREE_TYPE (atype))
>> +      && !init)
>
> This should check explicit_value_init._p rather than !init.  And also
> check zero_init_p.

Do you mean explicit_value_init_p && zero_init_p (atype)?  zero_init_p
doesn't sound like the correct thing to use here, because it doesn't
take into account whether a class array type has a constructor.  I am
probably misunderstanding the purpose of the zero_init_p check,
though.

-Nathan

Re: [PATCH][PR c++/82888] smarter code for default initialization of scalar arrays

[Jason Merrill]

On Fri, Nov 17, 2017 at 11:04 PM, Nathan Froyd <nfroyd@mozilla.com> wrote:
> On Fri, Nov 17, 2017 at 8:50 AM, Jason Merrill <jason@redhat.com> wrote:
>> On Thu, Nov 16, 2017 at 11:21 AM, Nathan Froyd <froydnj@mozilla.com> wrote:
>>> diff --git a/gcc/cp/init.c b/gcc/cp/init.c
>>> index c76460d..53d6133 100644
>>> --- a/gcc/cp/init.c
>>> +++ b/gcc/cp/init.c
>>> @@ -4038,6 +4038,15 @@ build_vec_init (tree base, tree maxindex, tree init,
>>>         }
>>>      }
>>>
>>> +  /* Default-initialize scalar arrays directly.  */
>>> +  if (TREE_CODE (atype) == ARRAY_TYPE
>>> +      && SCALAR_TYPE_P (TREE_TYPE (atype))
>>> +      && !init)
>>
>> This should check explicit_value_init._p rather than !init.  And also
>> check zero_init_p.
>
> Do you mean explicit_value_init_p && zero_init_p (atype)?

Yes.

> zero_init_p
> doesn't sound like the correct thing to use here, because it doesn't
> take into account whether a class array type has a constructor.  I am
> probably misunderstanding the purpose of the zero_init_p check,
> though.

Since you're already checking for scalar type, we don't need to worry
about classes.  The zero_init_p check is to handle pointers to data
members properly.

Jason

Re: [PATCH][PR c++/82888] smarter code for default initialization of scalar arrays

[Jason Merrill]

On Wed, Nov 29, 2017 at 4:30 PM, Jason Merrill <jason@redhat.com> wrote:
> On Fri, Nov 17, 2017 at 11:04 PM, Nathan Froyd <nfroyd@mozilla.com> wrote:
>> On Fri, Nov 17, 2017 at 8:50 AM, Jason Merrill <jason@redhat.com> wrote:
>>> On Thu, Nov 16, 2017 at 11:21 AM, Nathan Froyd <froydnj@mozilla.com> wrote:
>>>> diff --git a/gcc/cp/init.c b/gcc/cp/init.c
>>>> index c76460d..53d6133 100644
>>>> --- a/gcc/cp/init.c
>>>> +++ b/gcc/cp/init.c
>>>> @@ -4038,6 +4038,15 @@ build_vec_init (tree base, tree maxindex, tree init,
>>>>         }
>>>>      }
>>>>
>>>> +  /* Default-initialize scalar arrays directly.  */
>>>> +  if (TREE_CODE (atype) == ARRAY_TYPE
>>>> +      && SCALAR_TYPE_P (TREE_TYPE (atype))
>>>> +      && !init)
>>>
>>> This should check explicit_value_init._p rather than !init.  And also
>>> check zero_init_p.
>>
>> Do you mean explicit_value_init_p && zero_init_p (atype)?
>
> Yes.
>
>> zero_init_p
>> doesn't sound like the correct thing to use here, because it doesn't
>> take into account whether a class array type has a constructor.  I am
>> probably misunderstanding the purpose of the zero_init_p check,
>> though.
>
> Since you're already checking for scalar type, we don't need to worry
> about classes.  The zero_init_p check is to handle pointers to data
> members properly.

Any update?

Jason

Re: [PATCH][PR c++/82888] smarter code for default initialization of scalar arrays

[Jason Merrill]

On Thu, Dec 21, 2017 at 1:51 PM, Jason Merrill <jason@redhat.com> wrote:
> On Wed, Nov 29, 2017 at 4:30 PM, Jason Merrill <jason@redhat.com> wrote:
>> On Fri, Nov 17, 2017 at 11:04 PM, Nathan Froyd <nfroyd@mozilla.com> wrote:
>>> On Fri, Nov 17, 2017 at 8:50 AM, Jason Merrill <jason@redhat.com> wrote:
>>>> On Thu, Nov 16, 2017 at 11:21 AM, Nathan Froyd <froydnj@mozilla.com> wrote:
>>>>> diff --git a/gcc/cp/init.c b/gcc/cp/init.c
>>>>> index c76460d..53d6133 100644
>>>>> --- a/gcc/cp/init.c
>>>>> +++ b/gcc/cp/init.c
>>>>> @@ -4038,6 +4038,15 @@ build_vec_init (tree base, tree maxindex, tree init,
>>>>>         }
>>>>>      }
>>>>>
>>>>> +  /* Default-initialize scalar arrays directly.  */
>>>>> +  if (TREE_CODE (atype) == ARRAY_TYPE
>>>>> +      && SCALAR_TYPE_P (TREE_TYPE (atype))
>>>>> +      && !init)
>>>>
>>>> This should check explicit_value_init._p rather than !init.  And also
>>>> check zero_init_p.
>>>
>>> Do you mean explicit_value_init_p && zero_init_p (atype)?
>>
>> Yes.
>>
>>> zero_init_p
>>> doesn't sound like the correct thing to use here, because it doesn't
>>> take into account whether a class array type has a constructor.  I am
>>> probably misunderstanding the purpose of the zero_init_p check,
>>> though.
>>
>> Since you're already checking for scalar type, we don't need to worry
>> about classes.  The zero_init_p check is to handle pointers to data
>> members properly.
>
> Any update?

?

Re: [PATCH][PR c++/82888] smarter code for default initialization of scalar arrays

[Richard Biener]

On Thu, Nov 16, 2017 at 5:21 PM, Nathan Froyd <froydnj@mozilla.com> wrote:
> Default-initialization of scalar arrays in C++ member initialization
> lists produced rather slow code, laboriously setting each element of the
> array to zero.  It would be much faster to block-initialize the array,
> and that's what this patch does.
>
> The patch works for me, but I'm not sure if it's the best way to
> accomplish this.  At least two other possibilities come to mind:
>
> 1) Detect this case in build_vec_init_expr and act as though the user
>    wrote 'member{0}', which the front-end already produces efficient
>    code for.
>
> 2) Detect this case in build_vec_init, but again, act as though the user
>    wrote 'member{0}' and let everything proceed as normal.
>    (Alternatively, handle this case prior to calling build_vec_init and
>    pass different arguments to build_vec_init.)
>
> Opinions as to the best way forward here?  I'm unsure of whether the
> code below is front-end friendly; I see in the gimple dumps that the
> solution below adds an extra CLOBBER on 'this' for 'member()', whereas
> 'member{0}' does not.  It's possible that I'm missing something.
>
> Bootstrapped on x86_64-unknown-linux-gnu, no regressions.
>
> OK for trunk?

Ok. This is certainly the form the middle-end likes most.

Richard.

> -Nathan
>
> gcc/cp/
>         PR c++/82888
>         * init.c (build_vec_init): Handle default-initialization of array
>         types.
>
> gcc/testsuite/
>         PR c++/82888
>         * g++.dg/init/pr82888.C: New.
>
> diff --git a/gcc/cp/init.c b/gcc/cp/init.c
> index c76460d..53d6133 100644
> --- a/gcc/cp/init.c
> +++ b/gcc/cp/init.c
> @@ -4038,6 +4038,15 @@ build_vec_init (tree base, tree maxindex, tree init,
>         }
>      }
>
> +  /* Default-initialize scalar arrays directly.  */
> +  if (TREE_CODE (atype) == ARRAY_TYPE
> +      && SCALAR_TYPE_P (TREE_TYPE (atype))
> +      && !init)
> +    {
> +      gcc_assert (!from_array);
> +      return build2 (MODIFY_EXPR, atype, base, build_constructor (atype, NULL));
> +    }
> +
>    /* If we have a braced-init-list or string constant, make sure that the array
>       is big enough for all the initializers.  */
>    bool length_check = (init
> diff --git a/gcc/testsuite/g++.dg/init/pr82888.C b/gcc/testsuite/g++.dg/init/pr82888.C
> new file mode 100644
> index 0000000..9225e23
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/init/pr82888.C
> @@ -0,0 +1,18 @@
> +// { dg-do compile }
> +// { dg-options "-fdump-tree-gimple" }
> +
> +class A
> +{
> +public:
> +  A();
> +
> +private:
> +  unsigned char mStorage[4096];
> +};
> +
> +A::A()
> +  : mStorage()
> +{}
> +
> +// { dg-final { scan-tree-dump "this->mStorage = {}" "gimple" } }
> +// { dg-final { scan-tree-dump-not "&this->mStorage" "gimple" } }

[PATCH][PR c++/82888] smarter code for default initialization of scalar arrays

[Nathan Froyd]

Default-initialization of scalar arrays in C++ member initialization
lists produced rather slow code, laboriously setting each element of the
array to zero.  It would be much faster to block-initialize the array,
and that's what this patch does.

The patch works for me, but I'm not sure if it's the best way to
accomplish this.  At least two other possibilities come to mind:

1) Detect this case in build_vec_init_expr and act as though the user
   wrote 'member{0}', which the front-end already produces efficient
   code for.

2) Detect this case in build_vec_init, but again, act as though the user
   wrote 'member{0}' and let everything proceed as normal.
   (Alternatively, handle this case prior to calling build_vec_init and
   pass different arguments to build_vec_init.)

Opinions as to the best way forward here?  I'm unsure of whether the
code below is front-end friendly; I see in the gimple dumps that the
solution below adds an extra CLOBBER on 'this' for 'member()', whereas
'member{0}' does not.  It's possible that I'm missing something.

Bootstrapped on x86_64-unknown-linux-gnu, no regressions.

OK for trunk?

-Nathan

gcc/cp/
	PR c++/82888
	* init.c (build_vec_init): Handle default-initialization of array
	types.

gcc/testsuite/
	PR c++/82888
	* g++.dg/init/pr82888.C: New.

diff --git a/gcc/cp/init.c b/gcc/cp/init.c
index c76460d..53d6133 100644
--- a/gcc/cp/init.c
+++ b/gcc/cp/init.c
@@ -4038,6 +4038,15 @@ build_vec_init (tree base, tree maxindex, tree init,
 	}
     }
 
+  /* Default-initialize scalar arrays directly.  */
+  if (TREE_CODE (atype) == ARRAY_TYPE
+      && SCALAR_TYPE_P (TREE_TYPE (atype))
+      && !init)
+    {
+      gcc_assert (!from_array);
+      return build2 (MODIFY_EXPR, atype, base, build_constructor (atype, NULL));
+    }
+
   /* If we have a braced-init-list or string constant, make sure that the array
      is big enough for all the initializers.  */
   bool length_check = (init
diff --git a/gcc/testsuite/g++.dg/init/pr82888.C b/gcc/testsuite/g++.dg/init/pr82888.C
new file mode 100644
index 0000000..9225e23
--- /dev/null
+++ b/gcc/testsuite/g++.dg/init/pr82888.C
@@ -0,0 +1,18 @@
+// { dg-do compile }
+// { dg-options "-fdump-tree-gimple" }
+
+class A
+{
+public:
+  A();
+
+private:
+  unsigned char mStorage[4096];
+};
+
+A::A()
+  : mStorage()
+{}
+
+// { dg-final { scan-tree-dump "this->mStorage = {}" "gimple" } }
+// { dg-final { scan-tree-dump-not "&this->mStorage" "gimple" } }