Re: [PATCH][PR c++/82888] smarter code for default initialization of scalar arrays
On Thu, Dec 21, 2017 at 1:51 PM, Jason Merrillwrote: > On Wed, Nov 29, 2017 at 4:30 PM, Jason Merrill wrote: >> On Fri, Nov 17, 2017 at 11:04 PM, Nathan Froyd wrote: >>> On Fri, Nov 17, 2017 at 8:50 AM, Jason Merrill wrote: On Thu, Nov 16, 2017 at 11:21 AM, Nathan Froyd 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
On Wed, Nov 29, 2017 at 4:30 PM, Jason Merrillwrote: > On Fri, Nov 17, 2017 at 11:04 PM, Nathan Froyd wrote: >> On Fri, Nov 17, 2017 at 8:50 AM, Jason Merrill wrote: >>> On Thu, Nov 16, 2017 at 11:21 AM, Nathan Froyd 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
On Fri, Nov 17, 2017 at 11:04 PM, Nathan Froydwrote: > On Fri, Nov 17, 2017 at 8:50 AM, Jason Merrill wrote: >> On Thu, Nov 16, 2017 at 11:21 AM, Nathan Froyd 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
On Fri, Nov 17, 2017 at 8:50 AM, Jason Merrillwrote: > On Thu, Nov 16, 2017 at 11:21 AM, Nathan Froyd 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
On Thu, Nov 16, 2017 at 11:21 AM, Nathan Froydwrote: > 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
On Thu, Nov 16, 2017 at 5:21 PM, Nathan Froydwrote: > 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 000..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 ">mStorage" "gimple" } }
[PATCH][PR c++/82888] smarter code for default initialization of scalar arrays
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 000..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 ">mStorage" "gimple" } }