Re: [PING][PATCH] define auto_vec copy ctor and assignment (PR 90904)
On Fri, Aug 6, 2021 at 9:52 AM Christophe Lyon <
christophe.lyon@gmail.com> wrote:
>
>
> On Fri, Aug 6, 2021 at 4:07 AM Martin Sebor via Gcc-patches <
> gcc-patches@gcc.gnu.org> wrote:
>
>> On 7/30/21 9:06 AM, Jason Merrill wrote:
>> > On 7/27/21 2:56 PM, Martin Sebor wrote:
>> >> Ping: https://gcc.gnu.org/pipermail/gcc-patches/2021-July/575690.html
>> >>
>> >> Are there any other suggestions or comments or is the latest revision
>> >> okay to commit?
>> >
>> > OK.
>>
>> I had to make a few more adjustments to fix up code that's snuck
>> in since I last tested the patch. I committed r12-2776 after
>> retesting on x86_64-linux.
>>
>> With the cleanup out of the way I'll resubmit the copy ctor patch
>> next.
>>
>>
> Hi Martin,
>
> Your patch breaks the aarch64 build:
>
> /tmp/1784440_6.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/config/aarch64/aarch64-sve-builtins.cc:
> In function 'void aarch64_sve::register_svpattern()':
> /tmp/1784440_6.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/config/aarch64/aarch64-sve-builtins.cc:3502:27:
> error: use of deleted function 'vec::vec(auto_vec&) [with long
> unsigned int N = 32ul;
> T = std::pair]'
> "svpattern", values);
>^
> In file included from
> /tmp/1784440_6.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/hash-table.h:248:0,
> from
> /tmp/1784440_6.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/coretypes.h:480,
> from
> /tmp/1784440_6.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/config/aarch64/aarch64-sve-builtins.cc:24:
> /tmp/1784440_6.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/vec.h:1455:3:
> error: declared here
>vec (auto_vec &) = delete;
>^
> /tmp/1784440_6.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/config/aarch64/aarch64-sve-builtins.cc:
> In function 'void aarch64_sve::register_svprfop()':
> /tmp/1784440_6.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/config/aarch64/aarch64-sve-builtins.cc:3516:30:
> error: use of deleted function 'vec::vec(auto_vec&) [with long
> unsigned int N = 16ul;
> T = std::pair]'
> "svprfop", values);
> ^
> In file included from
> /tmp/1784440_6.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/hash-table.h:248:0,
> from
> /tmp/1784440_6.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/coretypes.h:480,
> from
> /tmp/1784440_6.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/config/aarch64/aarch64-sve-builtins.cc:24:
> /tmp/1784440_6.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/vec.h:1455:3:
> error: declared here
>vec (auto_vec &) = delete;
>^
>
> Can you check?
>
> Thanks,
>
This has now been fixed by Tamar, thanks!
Christophe
>
> Christophe
>
>>
>> Martin
>>
>> >
>> >> On 7/20/21 12:34 PM, Martin Sebor wrote:
>> >>> On 7/14/21 10:23 AM, Jason Merrill wrote:
>> On 7/14/21 10:46 AM, Martin Sebor wrote:
>> > On 7/13/21 9:39 PM, Jason Merrill wrote:
>> >> On 7/13/21 4:02 PM, Martin Sebor wrote:
>> >>> On 7/13/21 12:37 PM, Jason Merrill wrote:
>> On 7/13/21 10:08 AM, Jonathan Wakely wrote:
>> > On Mon, 12 Jul 2021 at 12:02, Richard Biener wrote:
>> >> Somebody with more C++ knowledge than me needs to approve the
>> >> vec.h changes - I don't feel competent to assess all effects
>> >> of the change.
>> >
>> > They look OK to me except for:
>> >
>> > -extern vnull vNULL;
>> > +static constexpr vnull vNULL{ };
>> >
>> > Making vNULL have static linkage can make it an ODR violation
>> > to use
>> > vNULL in templates and inline functions, because different
>> > instantiations will refer to a different "vNULL" in each
>> > translation
>> > unit.
>>
>> The ODR says this is OK because it's a literal constant with the
>> same value (6.2/12.2.1).
>>
>> But it would be better without the explicit 'static'; then in
>> C++17 it's implicitly inline instead of static.
>> >>>
>> >>> I'll remove the static.
>> >>>
>>
>> But then, do we really want to keep vNULL at all? It's a weird
>> blurring of the object/pointer boundary that is also dependent
>> on vec being a thin wrapper around a pointer. In almost all
>> cases it can be replaced with {}; one exception is ==
>> comparison, where it seems to be testing that the embedded
>> pointer is null, which is a weird thing to want to test.
>> >>>
>> >>> The one use case I know of for vNULL where I can't think of
>> >>> an equally good substitute is in passing a vec as an argument by
>> >>> value. The only way to do that that I can think of is to name
>> >>> the full vec type (i.e., the specialization) which is more typing
>> >>> and less generic than vNULL. I don't use vNULL myself so I
>> wouldn't
>> >>> miss this trick if
Re: [PING][PATCH] define auto_vec copy ctor and assignment (PR 90904)
On Fri, Aug 6, 2021 at 4:07 AM Martin Sebor via Gcc-patches <
gcc-patches@gcc.gnu.org> wrote:
> On 7/30/21 9:06 AM, Jason Merrill wrote:
> > On 7/27/21 2:56 PM, Martin Sebor wrote:
> >> Ping: https://gcc.gnu.org/pipermail/gcc-patches/2021-July/575690.html
> >>
> >> Are there any other suggestions or comments or is the latest revision
> >> okay to commit?
> >
> > OK.
>
> I had to make a few more adjustments to fix up code that's snuck
> in since I last tested the patch. I committed r12-2776 after
> retesting on x86_64-linux.
>
> With the cleanup out of the way I'll resubmit the copy ctor patch
> next.
>
>
Hi Martin,
Your patch breaks the aarch64 build:
/tmp/1784440_6.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/config/aarch64/aarch64-sve-builtins.cc:
In function 'void aarch64_sve::register_svpattern()':
/tmp/1784440_6.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/config/aarch64/aarch64-sve-builtins.cc:3502:27:
error: use of deleted function 'vec::vec(auto_vec&) [with long
unsigned int N = 32ul;
T = std::pair]'
"svpattern", values);
^
In file included from
/tmp/1784440_6.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/hash-table.h:248:0,
from
/tmp/1784440_6.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/coretypes.h:480,
from
/tmp/1784440_6.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/config/aarch64/aarch64-sve-builtins.cc:24:
/tmp/1784440_6.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/vec.h:1455:3:
error: declared here
vec (auto_vec &) = delete;
^
/tmp/1784440_6.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/config/aarch64/aarch64-sve-builtins.cc:
In function 'void aarch64_sve::register_svprfop()':
/tmp/1784440_6.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/config/aarch64/aarch64-sve-builtins.cc:3516:30:
error: use of deleted function 'vec::vec(auto_vec&) [with long
unsigned int N = 16ul;
T = std::pair]'
"svprfop", values);
^
In file included from
/tmp/1784440_6.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/hash-table.h:248:0,
from
/tmp/1784440_6.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/coretypes.h:480,
from
/tmp/1784440_6.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/config/aarch64/aarch64-sve-builtins.cc:24:
/tmp/1784440_6.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/vec.h:1455:3:
error: declared here
vec (auto_vec &) = delete;
^
Can you check?
Thanks,
Christophe
>
> Martin
>
> >
> >> On 7/20/21 12:34 PM, Martin Sebor wrote:
> >>> On 7/14/21 10:23 AM, Jason Merrill wrote:
> On 7/14/21 10:46 AM, Martin Sebor wrote:
> > On 7/13/21 9:39 PM, Jason Merrill wrote:
> >> On 7/13/21 4:02 PM, Martin Sebor wrote:
> >>> On 7/13/21 12:37 PM, Jason Merrill wrote:
> On 7/13/21 10:08 AM, Jonathan Wakely wrote:
> > On Mon, 12 Jul 2021 at 12:02, Richard Biener wrote:
> >> Somebody with more C++ knowledge than me needs to approve the
> >> vec.h changes - I don't feel competent to assess all effects
> >> of the change.
> >
> > They look OK to me except for:
> >
> > -extern vnull vNULL;
> > +static constexpr vnull vNULL{ };
> >
> > Making vNULL have static linkage can make it an ODR violation
> > to use
> > vNULL in templates and inline functions, because different
> > instantiations will refer to a different "vNULL" in each
> > translation
> > unit.
>
> The ODR says this is OK because it's a literal constant with the
> same value (6.2/12.2.1).
>
> But it would be better without the explicit 'static'; then in
> C++17 it's implicitly inline instead of static.
> >>>
> >>> I'll remove the static.
> >>>
>
> But then, do we really want to keep vNULL at all? It's a weird
> blurring of the object/pointer boundary that is also dependent
> on vec being a thin wrapper around a pointer. In almost all
> cases it can be replaced with {}; one exception is ==
> comparison, where it seems to be testing that the embedded
> pointer is null, which is a weird thing to want to test.
> >>>
> >>> The one use case I know of for vNULL where I can't think of
> >>> an equally good substitute is in passing a vec as an argument by
> >>> value. The only way to do that that I can think of is to name
> >>> the full vec type (i.e., the specialization) which is more typing
> >>> and less generic than vNULL. I don't use vNULL myself so I
> wouldn't
> >>> miss this trick if it were to be removed but others might feel
> >>> differently.
> >>
> >> In C++11, it can be replaced by {} in that context as well.
> >
> > Cool. I thought I'd tried { } here but I guess not.
> >
> >>
> >>> If not, I'm all for getting rid of vNULL but with over 350 uses
> >>>
Re: [PING][PATCH] define auto_vec copy ctor and assignment (PR 90904)
On 7/30/21 9:06 AM, Jason Merrill wrote:
On 7/27/21 2:56 PM, Martin Sebor wrote:
Ping: https://gcc.gnu.org/pipermail/gcc-patches/2021-July/575690.html
Are there any other suggestions or comments or is the latest revision
okay to commit?
OK.
I had to make a few more adjustments to fix up code that's snuck
in since I last tested the patch. I committed r12-2776 after
retesting on x86_64-linux.
With the cleanup out of the way I'll resubmit the copy ctor patch
next.
Martin
On 7/20/21 12:34 PM, Martin Sebor wrote:
On 7/14/21 10:23 AM, Jason Merrill wrote:
On 7/14/21 10:46 AM, Martin Sebor wrote:
On 7/13/21 9:39 PM, Jason Merrill wrote:
On 7/13/21 4:02 PM, Martin Sebor wrote:
On 7/13/21 12:37 PM, Jason Merrill wrote:
On 7/13/21 10:08 AM, Jonathan Wakely wrote:
On Mon, 12 Jul 2021 at 12:02, Richard Biener wrote:
Somebody with more C++ knowledge than me needs to approve the
vec.h changes - I don't feel competent to assess all effects
of the change.
They look OK to me except for:
-extern vnull vNULL;
+static constexpr vnull vNULL{ };
Making vNULL have static linkage can make it an ODR violation
to use
vNULL in templates and inline functions, because different
instantiations will refer to a different "vNULL" in each
translation
unit.
The ODR says this is OK because it's a literal constant with the
same value (6.2/12.2.1).
But it would be better without the explicit 'static'; then in
C++17 it's implicitly inline instead of static.
I'll remove the static.
But then, do we really want to keep vNULL at all? It's a weird
blurring of the object/pointer boundary that is also dependent
on vec being a thin wrapper around a pointer. In almost all
cases it can be replaced with {}; one exception is ==
comparison, where it seems to be testing that the embedded
pointer is null, which is a weird thing to want to test.
The one use case I know of for vNULL where I can't think of
an equally good substitute is in passing a vec as an argument by
value. The only way to do that that I can think of is to name
the full vec type (i.e., the specialization) which is more typing
and less generic than vNULL. I don't use vNULL myself so I wouldn't
miss this trick if it were to be removed but others might feel
differently.
In C++11, it can be replaced by {} in that context as well.
Cool. I thought I'd tried { } here but I guess not.
If not, I'm all for getting rid of vNULL but with over 350 uses
of it left, unless there's some clever trick to make the removal
(mostly) effortless and seamless, I'd much rather do it
independently
of this initial change. I also don't know if I can commit to making
all this cleanup.
I already have a patch to replace all but one use of vNULL, but
I'll hold off with it until after your patch.
So what's the next step? The patch only removes a few uses of vNULL
but doesn't add any. Is it good to go as is (without the static and
with the additional const changes Richard suggested)? This patch is
attached to my reply to Richard:
https://gcc.gnu.org/pipermail/gcc-patches/2021-July/575199.html
As Richard wrote:
The pieces where you change vec<> passing to const vec<>& and the few
where you change vec<> * to const vec<> * are OK - this should make
the
rest a smaller piece to review.
Please go ahead and apply those changes and send a new patch with
the remainder of the changes.
I have just pushed r12-2418:
https://gcc.gnu.org/pipermail/gcc-cvs/2021-July/350886.html
A few other comments:
- omp_declare_simd_clauses);
+ *omp_declare_simd_clauses);
Instead of doing this indirection in all of the callers, let's
change c_finish_omp_declare_simd to take a pointer as well, and do
the indirection in initializing a reference variable at the top of
the function.
Okay.
+ sched_init_luids (bbs.to_vec ());
+ haifa_init_h_i_d (bbs.to_vec ());
Why are these to_vec changes needed when you are also changing the
functions to take const&?
Calling to_vec() here isn't necessary so I've removed it.
- vec checks = LOOP_VINFO_CHECK_NONZERO (loop_vinfo);
+ vec checks = LOOP_VINFO_CHECK_NONZERO (loop_vinfo).to_vec ();
Why not use a reference here and in other similar spots?
Sure, that works too.
Attached is what's left of the original changes now that r12-2418
has been applied.
Martin
Re: [PING][PATCH] define auto_vec copy ctor and assignment (PR 90904)
On 7/27/21 2:56 PM, Martin Sebor wrote:
Ping: https://gcc.gnu.org/pipermail/gcc-patches/2021-July/575690.html
Are there any other suggestions or comments or is the latest revision
okay to commit?
OK.
On 7/20/21 12:34 PM, Martin Sebor wrote:
On 7/14/21 10:23 AM, Jason Merrill wrote:
On 7/14/21 10:46 AM, Martin Sebor wrote:
On 7/13/21 9:39 PM, Jason Merrill wrote:
On 7/13/21 4:02 PM, Martin Sebor wrote:
On 7/13/21 12:37 PM, Jason Merrill wrote:
On 7/13/21 10:08 AM, Jonathan Wakely wrote:
On Mon, 12 Jul 2021 at 12:02, Richard Biener wrote:
Somebody with more C++ knowledge than me needs to approve the
vec.h changes - I don't feel competent to assess all effects of
the change.
They look OK to me except for:
-extern vnull vNULL;
+static constexpr vnull vNULL{ };
Making vNULL have static linkage can make it an ODR violation to
use
vNULL in templates and inline functions, because different
instantiations will refer to a different "vNULL" in each
translation
unit.
The ODR says this is OK because it's a literal constant with the
same value (6.2/12.2.1).
But it would be better without the explicit 'static'; then in
C++17 it's implicitly inline instead of static.
I'll remove the static.
But then, do we really want to keep vNULL at all? It's a weird
blurring of the object/pointer boundary that is also dependent on
vec being a thin wrapper around a pointer. In almost all cases
it can be replaced with {}; one exception is == comparison, where
it seems to be testing that the embedded pointer is null, which
is a weird thing to want to test.
The one use case I know of for vNULL where I can't think of
an equally good substitute is in passing a vec as an argument by
value. The only way to do that that I can think of is to name
the full vec type (i.e., the specialization) which is more typing
and less generic than vNULL. I don't use vNULL myself so I wouldn't
miss this trick if it were to be removed but others might feel
differently.
In C++11, it can be replaced by {} in that context as well.
Cool. I thought I'd tried { } here but I guess not.
If not, I'm all for getting rid of vNULL but with over 350 uses
of it left, unless there's some clever trick to make the removal
(mostly) effortless and seamless, I'd much rather do it independently
of this initial change. I also don't know if I can commit to making
all this cleanup.
I already have a patch to replace all but one use of vNULL, but
I'll hold off with it until after your patch.
So what's the next step? The patch only removes a few uses of vNULL
but doesn't add any. Is it good to go as is (without the static and
with the additional const changes Richard suggested)? This patch is
attached to my reply to Richard:
https://gcc.gnu.org/pipermail/gcc-patches/2021-July/575199.html
As Richard wrote:
The pieces where you change vec<> passing to const vec<>& and the few
where you change vec<> * to const vec<> * are OK - this should make the
rest a smaller piece to review.
Please go ahead and apply those changes and send a new patch with the
remainder of the changes.
I have just pushed r12-2418:
https://gcc.gnu.org/pipermail/gcc-cvs/2021-July/350886.html
A few other comments:
- omp_declare_simd_clauses);
+ *omp_declare_simd_clauses);
Instead of doing this indirection in all of the callers, let's change
c_finish_omp_declare_simd to take a pointer as well, and do the
indirection in initializing a reference variable at the top of the
function.
Okay.
+ sched_init_luids (bbs.to_vec ());
+ haifa_init_h_i_d (bbs.to_vec ());
Why are these to_vec changes needed when you are also changing the
functions to take const&?
Calling to_vec() here isn't necessary so I've removed it.
- vec checks = LOOP_VINFO_CHECK_NONZERO (loop_vinfo);
+ vec checks = LOOP_VINFO_CHECK_NONZERO (loop_vinfo).to_vec ();
Why not use a reference here and in other similar spots?
Sure, that works too.
Attached is what's left of the original changes now that r12-2418
has been applied.
Martin
Re: [PING][PATCH] define auto_vec copy ctor and assignment (PR 90904)
Ping: https://gcc.gnu.org/pipermail/gcc-patches/2021-July/575690.html
Are there any other suggestions or comments or is the latest revision
okay to commit?
On 7/20/21 12:34 PM, Martin Sebor wrote:
On 7/14/21 10:23 AM, Jason Merrill wrote:
On 7/14/21 10:46 AM, Martin Sebor wrote:
On 7/13/21 9:39 PM, Jason Merrill wrote:
On 7/13/21 4:02 PM, Martin Sebor wrote:
On 7/13/21 12:37 PM, Jason Merrill wrote:
On 7/13/21 10:08 AM, Jonathan Wakely wrote:
On Mon, 12 Jul 2021 at 12:02, Richard Biener wrote:
Somebody with more C++ knowledge than me needs to approve the
vec.h changes - I don't feel competent to assess all effects of
the change.
They look OK to me except for:
-extern vnull vNULL;
+static constexpr vnull vNULL{ };
Making vNULL have static linkage can make it an ODR violation to use
vNULL in templates and inline functions, because different
instantiations will refer to a different "vNULL" in each translation
unit.
The ODR says this is OK because it's a literal constant with the
same value (6.2/12.2.1).
But it would be better without the explicit 'static'; then in
C++17 it's implicitly inline instead of static.
I'll remove the static.
But then, do we really want to keep vNULL at all? It's a weird
blurring of the object/pointer boundary that is also dependent on
vec being a thin wrapper around a pointer. In almost all cases it
can be replaced with {}; one exception is == comparison, where it
seems to be testing that the embedded pointer is null, which is a
weird thing to want to test.
The one use case I know of for vNULL where I can't think of
an equally good substitute is in passing a vec as an argument by
value. The only way to do that that I can think of is to name
the full vec type (i.e., the specialization) which is more typing
and less generic than vNULL. I don't use vNULL myself so I wouldn't
miss this trick if it were to be removed but others might feel
differently.
In C++11, it can be replaced by {} in that context as well.
Cool. I thought I'd tried { } here but I guess not.
If not, I'm all for getting rid of vNULL but with over 350 uses
of it left, unless there's some clever trick to make the removal
(mostly) effortless and seamless, I'd much rather do it independently
of this initial change. I also don't know if I can commit to making
all this cleanup.
I already have a patch to replace all but one use of vNULL, but I'll
hold off with it until after your patch.
So what's the next step? The patch only removes a few uses of vNULL
but doesn't add any. Is it good to go as is (without the static and
with the additional const changes Richard suggested)? This patch is
attached to my reply to Richard:
https://gcc.gnu.org/pipermail/gcc-patches/2021-July/575199.html
As Richard wrote:
The pieces where you change vec<> passing to const vec<>& and the few
where you change vec<> * to const vec<> * are OK - this should make the
rest a smaller piece to review.
Please go ahead and apply those changes and send a new patch with the
remainder of the changes.
I have just pushed r12-2418:
https://gcc.gnu.org/pipermail/gcc-cvs/2021-July/350886.html
A few other comments:
- omp_declare_simd_clauses);
+ *omp_declare_simd_clauses);
Instead of doing this indirection in all of the callers, let's change
c_finish_omp_declare_simd to take a pointer as well, and do the
indirection in initializing a reference variable at the top of the
function.
Okay.
+ sched_init_luids (bbs.to_vec ());
+ haifa_init_h_i_d (bbs.to_vec ());
Why are these to_vec changes needed when you are also changing the
functions to take const&?
Calling to_vec() here isn't necessary so I've removed it.
- vec checks = LOOP_VINFO_CHECK_NONZERO (loop_vinfo);
+ vec checks = LOOP_VINFO_CHECK_NONZERO (loop_vinfo).to_vec ();
Why not use a reference here and in other similar spots?
Sure, that works too.
Attached is what's left of the original changes now that r12-2418
has been applied.
Martin
Re: [PING][PATCH] define auto_vec copy ctor and assignment (PR 90904)
On 7/20/21 2:08 PM, Jason Merrill wrote:
On 7/20/21 2:34 PM, Martin Sebor wrote:
On 7/14/21 10:23 AM, Jason Merrill wrote:
On 7/14/21 10:46 AM, Martin Sebor wrote:
On 7/13/21 9:39 PM, Jason Merrill wrote:
On 7/13/21 4:02 PM, Martin Sebor wrote:
On 7/13/21 12:37 PM, Jason Merrill wrote:
On 7/13/21 10:08 AM, Jonathan Wakely wrote:
On Mon, 12 Jul 2021 at 12:02, Richard Biener wrote:
Somebody with more C++ knowledge than me needs to approve the
vec.h changes - I don't feel competent to assess all effects of
the change.
They look OK to me except for:
-extern vnull vNULL;
+static constexpr vnull vNULL{ };
Making vNULL have static linkage can make it an ODR violation to
use
vNULL in templates and inline functions, because different
instantiations will refer to a different "vNULL" in each
translation
unit.
The ODR says this is OK because it's a literal constant with the
same value (6.2/12.2.1).
But it would be better without the explicit 'static'; then in
C++17 it's implicitly inline instead of static.
I'll remove the static.
But then, do we really want to keep vNULL at all? It's a weird
blurring of the object/pointer boundary that is also dependent on
vec being a thin wrapper around a pointer. In almost all cases
it can be replaced with {}; one exception is == comparison, where
it seems to be testing that the embedded pointer is null, which
is a weird thing to want to test.
The one use case I know of for vNULL where I can't think of
an equally good substitute is in passing a vec as an argument by
value. The only way to do that that I can think of is to name
the full vec type (i.e., the specialization) which is more typing
and less generic than vNULL. I don't use vNULL myself so I wouldn't
miss this trick if it were to be removed but others might feel
differently.
In C++11, it can be replaced by {} in that context as well.
Cool. I thought I'd tried { } here but I guess not.
If not, I'm all for getting rid of vNULL but with over 350 uses
of it left, unless there's some clever trick to make the removal
(mostly) effortless and seamless, I'd much rather do it independently
of this initial change. I also don't know if I can commit to making
all this cleanup.
I already have a patch to replace all but one use of vNULL, but
I'll hold off with it until after your patch.
So what's the next step? The patch only removes a few uses of vNULL
but doesn't add any. Is it good to go as is (without the static and
with the additional const changes Richard suggested)? This patch is
attached to my reply to Richard:
https://gcc.gnu.org/pipermail/gcc-patches/2021-July/575199.html
As Richard wrote:
The pieces where you change vec<> passing to const vec<>& and the few
where you change vec<> * to const vec<> * are OK - this should make the
rest a smaller piece to review.
Please go ahead and apply those changes and send a new patch with the
remainder of the changes.
I have just pushed r12-2418:
https://gcc.gnu.org/pipermail/gcc-cvs/2021-July/350886.html
A few other comments:
- omp_declare_simd_clauses);
+ *omp_declare_simd_clauses);
Instead of doing this indirection in all of the callers, let's change
c_finish_omp_declare_simd to take a pointer as well, and do the
indirection in initializing a reference variable at the top of the
function.
Okay.
+ sched_init_luids (bbs.to_vec ());
+ haifa_init_h_i_d (bbs.to_vec ());
Why are these to_vec changes needed when you are also changing the
functions to take const&?
Calling to_vec() here isn't necessary so I've removed it.
- vec checks = LOOP_VINFO_CHECK_NONZERO (loop_vinfo);
+ vec checks = LOOP_VINFO_CHECK_NONZERO (loop_vinfo).to_vec ();
Why not use a reference here and in other similar spots?
Sure, that works too.
Attached is what's left of the original changes now that r12-2418
has been applied.
@@ -3364,7 +3364,8 @@ static void
vect_check_lower_bound (loop_vec_info loop_vinfo, tree expr, bool
unsigned_p,
poly_uint64 min_value)
{
- vec lower_bounds = LOOP_VINFO_LOWER_BOUNDS
(loop_vinfo);
+ vec lower_bounds
+ = LOOP_VINFO_LOWER_BOUNDS (loop_vinfo).to_vec ();
for (unsigned int i = 0; i < lower_bounds.length (); ++i)
if (operand_equal_p (lower_bounds[i].expr, expr, 0))
{
@@ -3466,7 +3467,7 @@ vect_prune_runtime_alias_test_list
(loop_vec_info loop_vinfo)
typedef pair_hash
tree_pair_hash;
hash_set compared_objects;
- vec may_alias_ddrs = LOOP_VINFO_MAY_ALIAS_DDRS (loop_vinfo);
+ vec may_alias_ddrs = LOOP_VINFO_MAY_ALIAS_DDRS
(loop_vinfo).to_vec ();
These could also be references.
Even const references it turns out for some of them.
That leaves this as the only remaining use of to_vec:
ipa_call_arg_values (ipa_auto_call_arg_values *aavals)
- : m_known_vals (aavals->m_known_vals),
- m_known_contexts (aavals->m_known_contexts),
- m_known_aggs (aavals->m_known_aggs),
-
Re: [PING][PATCH] define auto_vec copy ctor and assignment (PR 90904)
On 7/20/21 2:34 PM, Martin Sebor wrote:
On 7/14/21 10:23 AM, Jason Merrill wrote:
On 7/14/21 10:46 AM, Martin Sebor wrote:
On 7/13/21 9:39 PM, Jason Merrill wrote:
On 7/13/21 4:02 PM, Martin Sebor wrote:
On 7/13/21 12:37 PM, Jason Merrill wrote:
On 7/13/21 10:08 AM, Jonathan Wakely wrote:
On Mon, 12 Jul 2021 at 12:02, Richard Biener wrote:
Somebody with more C++ knowledge than me needs to approve the
vec.h changes - I don't feel competent to assess all effects of
the change.
They look OK to me except for:
-extern vnull vNULL;
+static constexpr vnull vNULL{ };
Making vNULL have static linkage can make it an ODR violation to use
vNULL in templates and inline functions, because different
instantiations will refer to a different "vNULL" in each translation
unit.
The ODR says this is OK because it's a literal constant with the
same value (6.2/12.2.1).
But it would be better without the explicit 'static'; then in
C++17 it's implicitly inline instead of static.
I'll remove the static.
But then, do we really want to keep vNULL at all? It's a weird
blurring of the object/pointer boundary that is also dependent on
vec being a thin wrapper around a pointer. In almost all cases it
can be replaced with {}; one exception is == comparison, where it
seems to be testing that the embedded pointer is null, which is a
weird thing to want to test.
The one use case I know of for vNULL where I can't think of
an equally good substitute is in passing a vec as an argument by
value. The only way to do that that I can think of is to name
the full vec type (i.e., the specialization) which is more typing
and less generic than vNULL. I don't use vNULL myself so I wouldn't
miss this trick if it were to be removed but others might feel
differently.
In C++11, it can be replaced by {} in that context as well.
Cool. I thought I'd tried { } here but I guess not.
If not, I'm all for getting rid of vNULL but with over 350 uses
of it left, unless there's some clever trick to make the removal
(mostly) effortless and seamless, I'd much rather do it independently
of this initial change. I also don't know if I can commit to making
all this cleanup.
I already have a patch to replace all but one use of vNULL, but I'll
hold off with it until after your patch.
So what's the next step? The patch only removes a few uses of vNULL
but doesn't add any. Is it good to go as is (without the static and
with the additional const changes Richard suggested)? This patch is
attached to my reply to Richard:
https://gcc.gnu.org/pipermail/gcc-patches/2021-July/575199.html
As Richard wrote:
The pieces where you change vec<> passing to const vec<>& and the few
where you change vec<> * to const vec<> * are OK - this should make the
rest a smaller piece to review.
Please go ahead and apply those changes and send a new patch with the
remainder of the changes.
I have just pushed r12-2418:
https://gcc.gnu.org/pipermail/gcc-cvs/2021-July/350886.html
A few other comments:
- omp_declare_simd_clauses);
+ *omp_declare_simd_clauses);
Instead of doing this indirection in all of the callers, let's change
c_finish_omp_declare_simd to take a pointer as well, and do the
indirection in initializing a reference variable at the top of the
function.
Okay.
+ sched_init_luids (bbs.to_vec ());
+ haifa_init_h_i_d (bbs.to_vec ());
Why are these to_vec changes needed when you are also changing the
functions to take const&?
Calling to_vec() here isn't necessary so I've removed it.
- vec checks = LOOP_VINFO_CHECK_NONZERO (loop_vinfo);
+ vec checks = LOOP_VINFO_CHECK_NONZERO (loop_vinfo).to_vec ();
Why not use a reference here and in other similar spots?
Sure, that works too.
Attached is what's left of the original changes now that r12-2418
has been applied.
@@ -3364,7 +3364,8 @@ static void
vect_check_lower_bound (loop_vec_info loop_vinfo, tree expr, bool unsigned_p,
poly_uint64 min_value)
{
- vec lower_bounds = LOOP_VINFO_LOWER_BOUNDS (loop_vinfo);
+ vec lower_bounds
+= LOOP_VINFO_LOWER_BOUNDS (loop_vinfo).to_vec ();
for (unsigned int i = 0; i < lower_bounds.length (); ++i)
if (operand_equal_p (lower_bounds[i].expr, expr, 0))
{
@@ -3466,7 +3467,7 @@ vect_prune_runtime_alias_test_list (loop_vec_info
loop_vinfo)
typedef pair_hash tree_pair_hash;
hash_set compared_objects;
- vec may_alias_ddrs = LOOP_VINFO_MAY_ALIAS_DDRS (loop_vinfo);
+ vec may_alias_ddrs = LOOP_VINFO_MAY_ALIAS_DDRS (loop_vinfo).to_vec ();
These could also be references.
That leaves this as the only remaining use of to_vec:
ipa_call_arg_values (ipa_auto_call_arg_values *aavals)
-: m_known_vals (aavals->m_known_vals),
- m_known_contexts (aavals->m_known_contexts),
- m_known_aggs (aavals->m_known_aggs),
- m_known_value_ranges (aavals->m_known_value_ranges)
+: m_known_vals (aavals->m_known_vals.to_vec
Re: [PING][PATCH] define auto_vec copy ctor and assignment (PR 90904)
On 7/14/21 10:23 AM, Jason Merrill wrote:
On 7/14/21 10:46 AM, Martin Sebor wrote:
On 7/13/21 9:39 PM, Jason Merrill wrote:
On 7/13/21 4:02 PM, Martin Sebor wrote:
On 7/13/21 12:37 PM, Jason Merrill wrote:
On 7/13/21 10:08 AM, Jonathan Wakely wrote:
On Mon, 12 Jul 2021 at 12:02, Richard Biener wrote:
Somebody with more C++ knowledge than me needs to approve the
vec.h changes - I don't feel competent to assess all effects of
the change.
They look OK to me except for:
-extern vnull vNULL;
+static constexpr vnull vNULL{ };
Making vNULL have static linkage can make it an ODR violation to use
vNULL in templates and inline functions, because different
instantiations will refer to a different "vNULL" in each translation
unit.
The ODR says this is OK because it's a literal constant with the
same value (6.2/12.2.1).
But it would be better without the explicit 'static'; then in C++17
it's implicitly inline instead of static.
I'll remove the static.
But then, do we really want to keep vNULL at all? It's a weird
blurring of the object/pointer boundary that is also dependent on
vec being a thin wrapper around a pointer. In almost all cases it
can be replaced with {}; one exception is == comparison, where it
seems to be testing that the embedded pointer is null, which is a
weird thing to want to test.
The one use case I know of for vNULL where I can't think of
an equally good substitute is in passing a vec as an argument by
value. The only way to do that that I can think of is to name
the full vec type (i.e., the specialization) which is more typing
and less generic than vNULL. I don't use vNULL myself so I wouldn't
miss this trick if it were to be removed but others might feel
differently.
In C++11, it can be replaced by {} in that context as well.
Cool. I thought I'd tried { } here but I guess not.
If not, I'm all for getting rid of vNULL but with over 350 uses
of it left, unless there's some clever trick to make the removal
(mostly) effortless and seamless, I'd much rather do it independently
of this initial change. I also don't know if I can commit to making
all this cleanup.
I already have a patch to replace all but one use of vNULL, but I'll
hold off with it until after your patch.
So what's the next step? The patch only removes a few uses of vNULL
but doesn't add any. Is it good to go as is (without the static and
with the additional const changes Richard suggested)? This patch is
attached to my reply to Richard:
https://gcc.gnu.org/pipermail/gcc-patches/2021-July/575199.html
As Richard wrote:
The pieces where you change vec<> passing to const vec<>& and the few
where you change vec<> * to const vec<> * are OK - this should make the
rest a smaller piece to review.
Please go ahead and apply those changes and send a new patch with the
remainder of the changes.
I have just pushed r12-2418:
https://gcc.gnu.org/pipermail/gcc-cvs/2021-July/350886.html
A few other comments:
- omp_declare_simd_clauses);
+ *omp_declare_simd_clauses);
Instead of doing this indirection in all of the callers, let's change
c_finish_omp_declare_simd to take a pointer as well, and do the
indirection in initializing a reference variable at the top of the
function.
Okay.
+ sched_init_luids (bbs.to_vec ());
+ haifa_init_h_i_d (bbs.to_vec ());
Why are these to_vec changes needed when you are also changing the
functions to take const&?
Calling to_vec() here isn't necessary so I've removed it.
- vec checks = LOOP_VINFO_CHECK_NONZERO (loop_vinfo);
+ vec checks = LOOP_VINFO_CHECK_NONZERO (loop_vinfo).to_vec ();
Why not use a reference here and in other similar spots?
Sure, that works too.
Attached is what's left of the original changes now that r12-2418
has been applied.
Martin
Disable implicit conversion from auto_vec to vec.
gcc/c/ChangeLog:
* c-parser.c (c_finish_omp_declare_simd): Adjust to vec change.
(c_parser_omp_declare_simd): Same.
* c-tree.h (c_build_function_call_vec): Same.
* c-typeck.c (c_build_function_call_vec): Same.
gcc/ChangeLog:
* dominance.c (prune_bbs_to_update_dominators): Adjust to vec change.
(iterate_fix_dominators): Same.
* dominance.h (iterate_fix_dominators): Same.
* ipa-prop.h:
* tree-ssa-pre.c (insert_into_preds_of_block): Same.
* tree-vect-data-refs.c (vect_check_nonzero_value): Same.
(vect_enhance_data_refs_alignment): Same.
(vect_check_lower_bound): Same.
(vect_prune_runtime_alias_test_list): Same.
(vect_permute_store_chain): Same.
* tree-vect-slp-patterns.c (vect_normalize_conj_loc): Same.
* tree-vect-stmts.c (vect_create_vectorized_demotion_stmts): Same.
* tree-vectorizer.h (vect_permute_store_chain): Same.
* vec.c (test_init): New.
(vec_c_tests): Call test_init.
* vec.h (struct vnull): Simplify.
(auto_vec::to_vec): New member function.
(vl_ptr>::copy): Use value initialization.
diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c
index
Re: [PING][PATCH] define auto_vec copy ctor and assignment (PR 90904)
On 7/14/21 10:46 AM, Martin Sebor wrote:
On 7/13/21 9:39 PM, Jason Merrill wrote:
On 7/13/21 4:02 PM, Martin Sebor wrote:
On 7/13/21 12:37 PM, Jason Merrill wrote:
On 7/13/21 10:08 AM, Jonathan Wakely wrote:
On Mon, 12 Jul 2021 at 12:02, Richard Biener wrote:
Somebody with more C++ knowledge than me needs to approve the
vec.h changes - I don't feel competent to assess all effects of
the change.
They look OK to me except for:
-extern vnull vNULL;
+static constexpr vnull vNULL{ };
Making vNULL have static linkage can make it an ODR violation to use
vNULL in templates and inline functions, because different
instantiations will refer to a different "vNULL" in each translation
unit.
The ODR says this is OK because it's a literal constant with the
same value (6.2/12.2.1).
But it would be better without the explicit 'static'; then in C++17
it's implicitly inline instead of static.
I'll remove the static.
But then, do we really want to keep vNULL at all? It's a weird
blurring of the object/pointer boundary that is also dependent on
vec being a thin wrapper around a pointer. In almost all cases it
can be replaced with {}; one exception is == comparison, where it
seems to be testing that the embedded pointer is null, which is a
weird thing to want to test.
The one use case I know of for vNULL where I can't think of
an equally good substitute is in passing a vec as an argument by
value. The only way to do that that I can think of is to name
the full vec type (i.e., the specialization) which is more typing
and less generic than vNULL. I don't use vNULL myself so I wouldn't
miss this trick if it were to be removed but others might feel
differently.
In C++11, it can be replaced by {} in that context as well.
Cool. I thought I'd tried { } here but I guess not.
If not, I'm all for getting rid of vNULL but with over 350 uses
of it left, unless there's some clever trick to make the removal
(mostly) effortless and seamless, I'd much rather do it independently
of this initial change. I also don't know if I can commit to making
all this cleanup.
I already have a patch to replace all but one use of vNULL, but I'll
hold off with it until after your patch.
So what's the next step? The patch only removes a few uses of vNULL
but doesn't add any. Is it good to go as is (without the static and
with the additional const changes Richard suggested)? This patch is
attached to my reply to Richard:
https://gcc.gnu.org/pipermail/gcc-patches/2021-July/575199.html
As Richard wrote:
The pieces where you change vec<> passing to const vec<>& and the few
where you change vec<> * to const vec<> * are OK - this should make the
rest a smaller piece to review.
Please go ahead and apply those changes and send a new patch with the
remainder of the changes.
A few other comments:
- omp_declare_simd_clauses);
+ *omp_declare_simd_clauses);
Instead of doing this indirection in all of the callers, let's change
c_finish_omp_declare_simd to take a pointer as well, and do the
indirection in initializing a reference variable at the top of the function.
+sched_init_luids (bbs.to_vec ());
+haifa_init_h_i_d (bbs.to_vec ());
Why are these to_vec changes needed when you are also changing the
functions to take const&?
- vec checks = LOOP_VINFO_CHECK_NONZERO (loop_vinfo);
+ vec checks = LOOP_VINFO_CHECK_NONZERO (loop_vinfo).to_vec ();
Why not use a reference here and in other similar spots?
Jason
Re: [PING][PATCH] define auto_vec copy ctor and assignment (PR 90904)
On 7/13/21 9:39 PM, Jason Merrill wrote:
On 7/13/21 4:02 PM, Martin Sebor wrote:
On 7/13/21 12:37 PM, Jason Merrill wrote:
On 7/13/21 10:08 AM, Jonathan Wakely wrote:
On Mon, 12 Jul 2021 at 12:02, Richard Biener wrote:
Somebody with more C++ knowledge than me needs to approve the
vec.h changes - I don't feel competent to assess all effects of the
change.
They look OK to me except for:
-extern vnull vNULL;
+static constexpr vnull vNULL{ };
Making vNULL have static linkage can make it an ODR violation to use
vNULL in templates and inline functions, because different
instantiations will refer to a different "vNULL" in each translation
unit.
The ODR says this is OK because it's a literal constant with the same
value (6.2/12.2.1).
But it would be better without the explicit 'static'; then in C++17
it's implicitly inline instead of static.
I'll remove the static.
But then, do we really want to keep vNULL at all? It's a weird
blurring of the object/pointer boundary that is also dependent on vec
being a thin wrapper around a pointer. In almost all cases it can be
replaced with {}; one exception is == comparison, where it seems to
be testing that the embedded pointer is null, which is a weird thing
to want to test.
The one use case I know of for vNULL where I can't think of
an equally good substitute is in passing a vec as an argument by
value. The only way to do that that I can think of is to name
the full vec type (i.e., the specialization) which is more typing
and less generic than vNULL. I don't use vNULL myself so I wouldn't
miss this trick if it were to be removed but others might feel
differently.
In C++11, it can be replaced by {} in that context as well.
Cool. I thought I'd tried { } here but I guess not.
If not, I'm all for getting rid of vNULL but with over 350 uses
of it left, unless there's some clever trick to make the removal
(mostly) effortless and seamless, I'd much rather do it independently
of this initial change. I also don't know if I can commit to making
all this cleanup.
I already have a patch to replace all but one use of vNULL, but I'll
hold off with it until after your patch.
So what's the next step? The patch only removes a few uses of vNULL
but doesn't add any. Is it good to go as is (without the static and
with the additional const changes Richard suggested)? This patch is
attached to my reply to Richard:
https://gcc.gnu.org/pipermail/gcc-patches/2021-July/575199.html
Martin
Somewhat relatedly, use of vec variables or fields seems almost
always a mistake, as they need explicit .release() that could be
automatic with auto_vec, and is easy to forget. For instance, the
recursive call in get_all_loop_exits returns a vec that is never
released. And I see a couple of leaks in the C++ front end as well.
I agree. The challenge I ran into with changing vec fields is with
code that uses the vec member as a reference to auto_vec. This is
the case in gcc/ipa-prop.h, for instance. Those instances could
be changed to auto_vec references or pointers but again it's a more
intrusive change than the simple replacements I was planning to make
in this first iteration.
So in summary, I agree with the changes you suggest. Given their
scope I'd prefer not to make them in the same patch, and rather make
them at some point in the future when I or someone else has the time
and energy. I'm running out.
Oh, absolutely.
Jason
Re: [PING][PATCH] define auto_vec copy ctor and assignment (PR 90904)
On 7/12/21 5:02 AM, Richard Biener wrote: On Wed, Jul 7, 2021 at 4:37 PM Martin Sebor wrote: On 7/7/21 1:28 AM, Richard Biener wrote: On Tue, Jul 6, 2021 at 5:06 PM Martin Sebor wrote: Ping: https://gcc.gnu.org/pipermail/gcc-patches/2021-June/573968.html Any questions/suggestions on the final patch or is it okay to commit? I don't remember seeing one (aka saying "bootstrapped/tested, OK to commit?" or so) - and the link above doesn't have one. So, can you re-post it please? The patch is attached to the email above with the following text at the end: Attached is a revised patch with these changes (a superset of those I sent in response to Jason's question), tested on x86_64. I've also attached it to this reply. Thanks - I was confused about the pipermail way of referencing attachments ... The pieces where you change vec<> passing to const vec<>& and the few where you change vec<> * to const vec<> * are OK - this should make the rest a smaller piece to review. In general const correctness changes should be considered obvious (vec<> to const vec<>& passing isn't quite obvious so I acked the cases explicitely). Okay. I think the vec<> -> vec<>& cases would either benefit from constification of callers that make using const vec<>& not possible or from a change to pass array_slice<> (not array_slice<>&), noting that the vec<> contents are mutated but the vec<> size does not change. I've reviewed the patch and found a handful of instances I had missed where the vec& could be made const. The rest of the vec<> -> vec<>& changes are all to functions that modify the vec, either directly or by passing it to other functions that do. (I you see some you want me to double-check let me know.) As a reminder, there may still be APIs where an existing by-value or by-reference vec could be made const vec& if they don't have to be touched for other reasons (i.e., passing an auto_vec as an argument). Those should also be reviewed at some point. Somebody with more C++ knowledge than me needs to approve the vec.h changes - I don't feel competent to assess all effects of the change. Ack. Attached is an updated patch with a few of the vec& -> const vec& changes and the removal of the static specifier on vNULL. Martin Disable implicit conversion from auto_vec to vec. * c-common.c (c_build_shufflevector): Adjust to vec change. * c-common.h (c_build_shufflevector): Same. gcc/c/ChangeLog: * c-parser.c (c_finish_omp_declare_simd): Adjust to vec change. (c_parser_omp_declare_simd): Same. * c-tree.h (c_build_function_call_vec): Same. * c-typeck.c (c_build_function_call_vec): Same. gcc/ChangeLog: * cfgloop.h (single_likely_exit): Adjust to vec change. * cfgloopanal.c (single_likely_exit): Same. * cgraph.h (struct cgraph_node): Same. * cgraphclones.c (cgraph_node::create_virtual_clone): Same. * dominance.c (prune_bbs_to_update_dominators): Same. (iterate_fix_dominators): Same. * dominance.h (iterate_fix_dominators): Same. * genautomata.c (merge_states): Same. * genextract.c (VEC_char_to_string): Same. * genmatch.c (dt_node::gen_kids_1): Same. (walk_captures): Same. * gimple-ssa-store-merging.c (check_no_overlap): Same. * gimple.c (gimple_build_call_vec): Same. (gimple_build_call_internal_vec): Same. (gimple_build_switch): Same. (sort_case_labels): Same. (preprocess_case_label_vec_for_gimple): Same. * gimple.h (gimple_build_call_vec): Same. (gimple_build_call_internal_vec): Same. (gimple_build_switch): Same. (sort_case_labels): Same. (preprocess_case_label_vec_for_gimple): Same. * haifa-sched.c (calc_priorities): Same. (haifa_sched_init): Same. (sched_init_luids): Same. (haifa_init_h_i_d): Same. * ipa-cp.c (ipa_get_indirect_edge_target_1): Same. (adjust_callers_for_value_intersection): Same. (find_more_scalar_values_for_callers_subset): Same. (find_more_contexts_for_caller_subset): Same. (find_aggregate_values_for_callers_subset): Same. (copy_useful_known_contexts): Same. * ipa-fnsummary.c (remap_edge_summaries): Same. (remap_freqcounting_predicate): Same. * ipa-inline.c (add_new_edges_to_heap): Same. * ipa-predicate.c (predicate::remap_after_inlining): Same. * ipa-predicate.h: * ipa-prop.c (ipa_find_agg_cst_for_param): Same. * ipa-prop.h (ipa_find_agg_cst_for_param): Same. * ira-build.c (ira_loop_tree_body_rev_postorder): Same. * read-rtl.c (apply_iterators): Same. * rtl.h (native_decode_rtx): Same. (native_decode_vector_rtx): Same. * sched-int.h (sched_init_luids): Same. (haifa_init_h_i_d): Same. * simplify-rtx.c (native_decode_vector_rtx): Same. (native_decode_rtx): Same. * tree-call-cdce.c (gen_shrink_wrap_conditions): Same. (shrink_wrap_one_built_in_call_with_conds): Same. (shrink_wrap_conditional_dead_built_in_calls): Same. * tree-data-ref.c (create_runtime_alias_checks): Same. (compute_all_dependences): Same. * tree-data-ref.h (compute_all_dependences): Same. (create_runtime_alias_checks): Same. (index_in_loop_nest):
Re: [PING][PATCH] define auto_vec copy ctor and assignment (PR 90904)
On Wed, 14 Jul 2021 at 04:39, Jason Merrill wrote:
>
> On 7/13/21 4:02 PM, Martin Sebor wrote:
> > On 7/13/21 12:37 PM, Jason Merrill wrote:
> >> On 7/13/21 10:08 AM, Jonathan Wakely wrote:
> >>> On Mon, 12 Jul 2021 at 12:02, Richard Biener wrote:
> Somebody with more C++ knowledge than me needs to approve the
> vec.h changes - I don't feel competent to assess all effects of the
> change.
> >>>
> >>> They look OK to me except for:
> >>>
> >>> -extern vnull vNULL;
> >>> +static constexpr vnull vNULL{ };
> >>>
> >>> Making vNULL have static linkage can make it an ODR violation to use
> >>> vNULL in templates and inline functions, because different
> >>> instantiations will refer to a different "vNULL" in each translation
> >>> unit.
> >>
> >> The ODR says this is OK because it's a literal constant with the same
> >> value (6.2/12.2.1).
> >>
> >> But it would be better without the explicit 'static'; then in C++17
> >> it's implicitly inline instead of static.
> >
> > I'll remove the static.
> >
> >>
> >> But then, do we really want to keep vNULL at all? It's a weird
> >> blurring of the object/pointer boundary that is also dependent on vec
> >> being a thin wrapper around a pointer. In almost all cases it can be
> >> replaced with {}; one exception is == comparison, where it seems to be
> >> testing that the embedded pointer is null, which is a weird thing to
> >> want to test.
> >
> > The one use case I know of for vNULL where I can't think of
> > an equally good substitute is in passing a vec as an argument by
> > value. The only way to do that that I can think of is to name
> > the full vec type (i.e., the specialization) which is more typing
> > and less generic than vNULL. I don't use vNULL myself so I wouldn't
> > miss this trick if it were to be removed but others might feel
> > differently.
>
> In C++11, it can be replaced by {} in that context as well.
Or if people don't like that, you could add a constructor taking
std::nullptr_t and an equality comparison with std::nullptr_t and then
use nullptr instead of vNULL.
I think just using {} for an empty, value-initialized vec makes more
sense though.
Re: [PING][PATCH] define auto_vec copy ctor and assignment (PR 90904)
On 7/13/21 4:02 PM, Martin Sebor wrote:
On 7/13/21 12:37 PM, Jason Merrill wrote:
On 7/13/21 10:08 AM, Jonathan Wakely wrote:
On Mon, 12 Jul 2021 at 12:02, Richard Biener wrote:
Somebody with more C++ knowledge than me needs to approve the
vec.h changes - I don't feel competent to assess all effects of the
change.
They look OK to me except for:
-extern vnull vNULL;
+static constexpr vnull vNULL{ };
Making vNULL have static linkage can make it an ODR violation to use
vNULL in templates and inline functions, because different
instantiations will refer to a different "vNULL" in each translation
unit.
The ODR says this is OK because it's a literal constant with the same
value (6.2/12.2.1).
But it would be better without the explicit 'static'; then in C++17
it's implicitly inline instead of static.
I'll remove the static.
But then, do we really want to keep vNULL at all? It's a weird
blurring of the object/pointer boundary that is also dependent on vec
being a thin wrapper around a pointer. In almost all cases it can be
replaced with {}; one exception is == comparison, where it seems to be
testing that the embedded pointer is null, which is a weird thing to
want to test.
The one use case I know of for vNULL where I can't think of
an equally good substitute is in passing a vec as an argument by
value. The only way to do that that I can think of is to name
the full vec type (i.e., the specialization) which is more typing
and less generic than vNULL. I don't use vNULL myself so I wouldn't
miss this trick if it were to be removed but others might feel
differently.
In C++11, it can be replaced by {} in that context as well.
If not, I'm all for getting rid of vNULL but with over 350 uses
of it left, unless there's some clever trick to make the removal
(mostly) effortless and seamless, I'd much rather do it independently
of this initial change. I also don't know if I can commit to making
all this cleanup.
I already have a patch to replace all but one use of vNULL, but I'll
hold off with it until after your patch.
Somewhat relatedly, use of vec variables or fields seems almost
always a mistake, as they need explicit .release() that could be
automatic with auto_vec, and is easy to forget. For instance, the
recursive call in get_all_loop_exits returns a vec that is never
released. And I see a couple of leaks in the C++ front end as well.
I agree. The challenge I ran into with changing vec fields is with
code that uses the vec member as a reference to auto_vec. This is
the case in gcc/ipa-prop.h, for instance. Those instances could
be changed to auto_vec references or pointers but again it's a more
intrusive change than the simple replacements I was planning to make
in this first iteration.
So in summary, I agree with the changes you suggest. Given their
scope I'd prefer not to make them in the same patch, and rather make
them at some point in the future when I or someone else has the time
and energy. I'm running out.
Oh, absolutely.
Jason
Re: [PING][PATCH] define auto_vec copy ctor and assignment (PR 90904)
On 7/13/21 12:37 PM, Jason Merrill wrote:
On 7/13/21 10:08 AM, Jonathan Wakely wrote:
On Mon, 12 Jul 2021 at 12:02, Richard Biener wrote:
Somebody with more C++ knowledge than me needs to approve the
vec.h changes - I don't feel competent to assess all effects of the
change.
They look OK to me except for:
-extern vnull vNULL;
+static constexpr vnull vNULL{ };
Making vNULL have static linkage can make it an ODR violation to use
vNULL in templates and inline functions, because different
instantiations will refer to a different "vNULL" in each translation
unit.
The ODR says this is OK because it's a literal constant with the same
value (6.2/12.2.1).
But it would be better without the explicit 'static'; then in C++17 it's
implicitly inline instead of static.
I'll remove the static.
But then, do we really want to keep vNULL at all? It's a weird blurring
of the object/pointer boundary that is also dependent on vec being a
thin wrapper around a pointer. In almost all cases it can be replaced
with {}; one exception is == comparison, where it seems to be testing
that the embedded pointer is null, which is a weird thing to want to test.
The one use case I know of for vNULL where I can't think of
an equally good substitute is in passing a vec as an argument by
value. The only way to do that that I can think of is to name
the full vec type (i.e., the specialization) which is more typing
and less generic than vNULL. I don't use vNULL myself so I wouldn't
miss this trick if it were to be removed but others might feel
differently.
If not, I'm all for getting rid of vNULL but with over 350 uses
of it left, unless there's some clever trick to make the removal
(mostly) effortless and seamless, I'd much rather do it independently
of this initial change. I also don't know if I can commit to making
all this cleanup.
Somewhat relatedly, use of vec variables or fields seems almost
always a mistake, as they need explicit .release() that could be
automatic with auto_vec, and is easy to forget. For instance, the
recursive call in get_all_loop_exits returns a vec that is never
released. And I see a couple of leaks in the C++ front end as well.
I agree. The challenge I ran into with changing vec fields is with
code that uses the vec member as a reference to auto_vec. This is
the case in gcc/ipa-prop.h, for instance. Those instances could
be changed to auto_vec references or pointers but again it's a more
intrusive change than the simple replacements I was planning to make
in this first iteration.
So in summary, I agree with the changes you suggest. Given their
scope I'd prefer not to make them in the same patch, and rather make
them at some point in the future when I or someone else has the time
and energy. I'm running out.
Martin
Re: [PING][PATCH] define auto_vec copy ctor and assignment (PR 90904)
On 7/13/21 10:08 AM, Jonathan Wakely wrote:
On Mon, 12 Jul 2021 at 12:02, Richard Biener wrote:
Somebody with more C++ knowledge than me needs to approve the
vec.h changes - I don't feel competent to assess all effects of the change.
They look OK to me except for:
-extern vnull vNULL;
+static constexpr vnull vNULL{ };
Making vNULL have static linkage can make it an ODR violation to use
vNULL in templates and inline functions, because different
instantiations will refer to a different "vNULL" in each translation
unit.
The ODR says this is OK because it's a literal constant with the same
value (6.2/12.2.1).
But it would be better without the explicit 'static'; then in C++17 it's
implicitly inline instead of static.
But then, do we really want to keep vNULL at all? It's a weird blurring
of the object/pointer boundary that is also dependent on vec being a
thin wrapper around a pointer. In almost all cases it can be replaced
with {}; one exception is == comparison, where it seems to be testing
that the embedded pointer is null, which is a weird thing to want to test.
Somewhat relatedly, use of vec variables or fields seems almost
always a mistake, as they need explicit .release() that could be
automatic with auto_vec, and is easy to forget. For instance, the
recursive call in get_all_loop_exits returns a vec that is never
released. And I see a couple of leaks in the C++ front end as well.
Jason
Re: [PING][PATCH] define auto_vec copy ctor and assignment (PR 90904)
On Mon, 12 Jul 2021 at 12:02, Richard Biener wrote:
> Somebody with more C++ knowledge than me needs to approve the
> vec.h changes - I don't feel competent to assess all effects of the change.
They look OK to me except for:
-extern vnull vNULL;
+static constexpr vnull vNULL{ };
Making vNULL have static linkage can make it an ODR violation to use
vNULL in templates and inline functions, because different
instantiations will refer to a different "vNULL" in each translation
unit. The extern object avoids that problem. It probably won't cause
real problems in practice but it's still technically UB.
If avoiding the extern variable is desirable, you can make it inline
when compiled by a C++17 compiler (such as GCC 11):
#if __cpp_inline_variables
inline constexpr vnull vNULL{ };
#else
extern const vnull vNULL;
#endif
and then define it in vec.c:
#if ! __cpp_inline_variables
const vnull vNULL{ };
#endif
Re: [PING][PATCH] define auto_vec copy ctor and assignment (PR 90904)
On Wed, Jul 7, 2021 at 4:37 PM Martin Sebor wrote: > > On 7/7/21 1:28 AM, Richard Biener wrote: > > On Tue, Jul 6, 2021 at 5:06 PM Martin Sebor wrote: > >> > >> Ping: https://gcc.gnu.org/pipermail/gcc-patches/2021-June/573968.html > >> > >> Any questions/suggestions on the final patch or is it okay to commit? > > > > I don't remember seeing one (aka saying "bootstrapped/tested, OK to commit?" > > or so) - and the link above doesn't have one. > > > > So, can you re-post it please? > > The patch is attached to the email above with the following text > at the end: > >Attached is a revised patch with these changes (a superset of >those I sent in response to Jason's question), tested on x86_64. > > I've also attached it to this reply. Thanks - I was confused about the pipermail way of referencing attachments ... The pieces where you change vec<> passing to const vec<>& and the few where you change vec<> * to const vec<> * are OK - this should make the rest a smaller piece to review. In general const correctness changes should be considered obvious (vec<> to const vec<>& passing isn't quite obvious so I acked the cases explicitely). I think the vec<> -> vec<>& cases would either benefit from constification of callers that make using const vec<>& not possible or from a change to pass array_slice<> (not array_slice<>&), noting that the vec<> contents are mutated but the vec<> size does not change. Somebody with more C++ knowledge than me needs to approve the vec.h changes - I don't feel competent to assess all effects of the change. Thanks, Richard. > Martin > > > > > Thanks, > > Richard. > > > >> On 6/29/21 7:46 PM, Martin Sebor wrote: > >>> On 6/29/21 4:58 AM, Richard Biener wrote: > On Mon, Jun 28, 2021 at 8:07 PM Martin Sebor wrote: > > > > On 6/28/21 2:07 AM, Richard Biener wrote: > >> On Sat, Jun 26, 2021 at 12:36 AM Martin Sebor wrote: > >>> > >>> On 6/25/21 4:11 PM, Jason Merrill wrote: > On 6/25/21 4:51 PM, Martin Sebor wrote: > > On 6/1/21 3:38 PM, Jason Merrill wrote: > >> On 6/1/21 3:56 PM, Martin Sebor wrote: > >>> On 5/27/21 2:53 PM, Jason Merrill wrote: > On 4/27/21 11:52 AM, Martin Sebor via Gcc-patches wrote: > > On 4/27/21 8:04 AM, Richard Biener wrote: > >> On Tue, Apr 27, 2021 at 3:59 PM Martin Sebor > >> wrote: > >>> > >>> On 4/27/21 1:58 AM, Richard Biener wrote: > On Tue, Apr 27, 2021 at 2:46 AM Martin Sebor via Gcc-patches > wrote: > > > > PR 90904 notes that auto_vec is unsafe to copy and assign > > because > > the class manages its own memory but doesn't define (or > > delete) > > either special function. Since I first ran into the > > problem, > > auto_vec has grown a move ctor and move assignment from > > a dynamically-allocated vec but still no copy ctor or copy > > assignment operator. > > > > The attached patch adds the two special functions to > > auto_vec > > along > > with a few simple tests. It makes auto_vec safe to use in > > containers > > that expect copyable and assignable element types and passes > > bootstrap > > and regression testing on x86_64-linux. > > The question is whether we want such uses to appear since > those > can be quite inefficient? Thus the option is to delete those > operators? > >>> > >>> I would strongly prefer the generic vector class to have the > >>> properties > >>> expected of any other generic container: copyable and > >>> assignable. If > >>> we also want another vector type with this restriction I > >>> suggest > >>> to add > >>> another "noncopyable" type and make that property explicit in > >>> its name. > >>> I can submit one in a followup patch if you think we need one. > >> > >> I'm not sure (and not strictly against the copy and assign). > >> Looking around > >> I see that vec<> does not do deep copying. Making > >> auto_vec<> do it > >> might be surprising (I added the move capability to match > >> how vec<> > >> is used - as "reference" to a vector) > > > > The vec base classes are special: they have no ctors at all > > (because > > of their use in unions). That's something we might have to > > live with > > but it's not a model to follow in ordinary containers. >
Re: [PING][PATCH] define auto_vec copy ctor and assignment (PR 90904)
On 7/7/21 1:28 AM, Richard Biener wrote: On Tue, Jul 6, 2021 at 5:06 PM Martin Sebor wrote: Ping: https://gcc.gnu.org/pipermail/gcc-patches/2021-June/573968.html Any questions/suggestions on the final patch or is it okay to commit? I don't remember seeing one (aka saying "bootstrapped/tested, OK to commit?" or so) - and the link above doesn't have one. So, can you re-post it please? The patch is attached to the email above with the following text at the end: Attached is a revised patch with these changes (a superset of those I sent in response to Jason's question), tested on x86_64. I've also attached it to this reply. Martin Thanks, Richard. On 6/29/21 7:46 PM, Martin Sebor wrote: On 6/29/21 4:58 AM, Richard Biener wrote: On Mon, Jun 28, 2021 at 8:07 PM Martin Sebor wrote: On 6/28/21 2:07 AM, Richard Biener wrote: On Sat, Jun 26, 2021 at 12:36 AM Martin Sebor wrote: On 6/25/21 4:11 PM, Jason Merrill wrote: On 6/25/21 4:51 PM, Martin Sebor wrote: On 6/1/21 3:38 PM, Jason Merrill wrote: On 6/1/21 3:56 PM, Martin Sebor wrote: On 5/27/21 2:53 PM, Jason Merrill wrote: On 4/27/21 11:52 AM, Martin Sebor via Gcc-patches wrote: On 4/27/21 8:04 AM, Richard Biener wrote: On Tue, Apr 27, 2021 at 3:59 PM Martin Sebor wrote: On 4/27/21 1:58 AM, Richard Biener wrote: On Tue, Apr 27, 2021 at 2:46 AM Martin Sebor via Gcc-patches wrote: PR 90904 notes that auto_vec is unsafe to copy and assign because the class manages its own memory but doesn't define (or delete) either special function. Since I first ran into the problem, auto_vec has grown a move ctor and move assignment from a dynamically-allocated vec but still no copy ctor or copy assignment operator. The attached patch adds the two special functions to auto_vec along with a few simple tests. It makes auto_vec safe to use in containers that expect copyable and assignable element types and passes bootstrap and regression testing on x86_64-linux. The question is whether we want such uses to appear since those can be quite inefficient? Thus the option is to delete those operators? I would strongly prefer the generic vector class to have the properties expected of any other generic container: copyable and assignable. If we also want another vector type with this restriction I suggest to add another "noncopyable" type and make that property explicit in its name. I can submit one in a followup patch if you think we need one. I'm not sure (and not strictly against the copy and assign). Looking around I see that vec<> does not do deep copying. Making auto_vec<> do it might be surprising (I added the move capability to match how vec<> is used - as "reference" to a vector) The vec base classes are special: they have no ctors at all (because of their use in unions). That's something we might have to live with but it's not a model to follow in ordinary containers. I don't think we have to live with it anymore, now that we're writing C++11. The auto_vec class was introduced to fill the need for a conventional sequence container with a ctor and dtor. The missing copy ctor and assignment operators were an oversight, not a deliberate feature. This change fixes that oversight. The revised patch also adds a copy ctor/assignment to the auto_vec primary template (that's also missing it). In addition, it adds a new class called auto_vec_ncopy that disables copying and assignment as you prefer. Hmm, adding another class doesn't really help with the confusion richi mentions. And many uses of auto_vec will pass them as vec, which will still do a shallow copy. I think it's probably better to disable the copy special members for auto_vec until we fix vec<>. There are at least a couple of problems that get in the way of fixing all of vec to act like a well-behaved C++ container: 1) The embedded vec has a trailing "flexible" array member with its instances having different size. They're initialized by memset and copied by memcpy. The class can't have copy ctors or assignments but it should disable/delete them instead. 2) The heap-based vec is used throughout GCC with the assumption of shallow copy semantics (not just as function arguments but also as members of other such POD classes). This can be changed by providing copy and move ctors and assignment operators for it, and also for some of the classes in which it's a member and that are used with the same assumption. 3) The heap-based vec::block_remove() assumes its elements are PODs. That breaks in VEC_ORDERED_REMOVE_IF (used in gcc/dwarf2cfi.c:2862 and tree-vect-patterns.c). I took a stab at both and while (1) is easy, (2) is shaping up to be a big and tricky project. Tricky because it involves using std::move in places where what's moved is subsequently still used. I can keep plugging away at it but it won't change the fact that the embedded and heap-based vecs have different requirements. It doesn't seem to me that having a safely copyable auto_vec needs to
Re: [PING][PATCH] define auto_vec copy ctor and assignment (PR 90904)
On Tue, Jul 6, 2021 at 5:06 PM Martin Sebor wrote: > > Ping: https://gcc.gnu.org/pipermail/gcc-patches/2021-June/573968.html > > Any questions/suggestions on the final patch or is it okay to commit? I don't remember seeing one (aka saying "bootstrapped/tested, OK to commit?" or so) - and the link above doesn't have one. So, can you re-post it please? Thanks, Richard. > On 6/29/21 7:46 PM, Martin Sebor wrote: > > On 6/29/21 4:58 AM, Richard Biener wrote: > >> On Mon, Jun 28, 2021 at 8:07 PM Martin Sebor wrote: > >>> > >>> On 6/28/21 2:07 AM, Richard Biener wrote: > On Sat, Jun 26, 2021 at 12:36 AM Martin Sebor wrote: > > > > On 6/25/21 4:11 PM, Jason Merrill wrote: > >> On 6/25/21 4:51 PM, Martin Sebor wrote: > >>> On 6/1/21 3:38 PM, Jason Merrill wrote: > On 6/1/21 3:56 PM, Martin Sebor wrote: > > On 5/27/21 2:53 PM, Jason Merrill wrote: > >> On 4/27/21 11:52 AM, Martin Sebor via Gcc-patches wrote: > >>> On 4/27/21 8:04 AM, Richard Biener wrote: > On Tue, Apr 27, 2021 at 3:59 PM Martin Sebor > wrote: > > > > On 4/27/21 1:58 AM, Richard Biener wrote: > >> On Tue, Apr 27, 2021 at 2:46 AM Martin Sebor via Gcc-patches > >> wrote: > >>> > >>> PR 90904 notes that auto_vec is unsafe to copy and assign > >>> because > >>> the class manages its own memory but doesn't define (or > >>> delete) > >>> either special function. Since I first ran into the > >>> problem, > >>> auto_vec has grown a move ctor and move assignment from > >>> a dynamically-allocated vec but still no copy ctor or copy > >>> assignment operator. > >>> > >>> The attached patch adds the two special functions to > >>> auto_vec > >>> along > >>> with a few simple tests. It makes auto_vec safe to use in > >>> containers > >>> that expect copyable and assignable element types and passes > >>> bootstrap > >>> and regression testing on x86_64-linux. > >> > >> The question is whether we want such uses to appear since > >> those > >> can be quite inefficient? Thus the option is to delete those > >> operators? > > > > I would strongly prefer the generic vector class to have the > > properties > > expected of any other generic container: copyable and > > assignable. If > > we also want another vector type with this restriction I > > suggest > > to add > > another "noncopyable" type and make that property explicit in > > its name. > > I can submit one in a followup patch if you think we need one. > > I'm not sure (and not strictly against the copy and assign). > Looking around > I see that vec<> does not do deep copying. Making > auto_vec<> do it > might be surprising (I added the move capability to match > how vec<> > is used - as "reference" to a vector) > >>> > >>> The vec base classes are special: they have no ctors at all > >>> (because > >>> of their use in unions). That's something we might have to > >>> live with > >>> but it's not a model to follow in ordinary containers. > >> > >> I don't think we have to live with it anymore, now that we're > >> writing C++11. > >> > >>> The auto_vec class was introduced to fill the need for a > >>> conventional > >>> sequence container with a ctor and dtor. The missing copy > >>> ctor and > >>> assignment operators were an oversight, not a deliberate > >>> feature. > >>> This change fixes that oversight. > >>> > >>> The revised patch also adds a copy ctor/assignment to the > >>> auto_vec > >>> primary template (that's also missing it). In addition, it adds > >>> a new class called auto_vec_ncopy that disables copying and > >>> assignment as you prefer. > >> > >> Hmm, adding another class doesn't really help with the confusion > >> richi mentions. And many uses of auto_vec will pass them as vec, > >> which will still do a shallow copy. I think it's probably better > >> to disable the copy special members for auto_vec until we fix > >> vec<>. > > > > There are at least a couple of problems that get in the way of > > fixing > > all of vec to act like a well-behaved C++ container: > > > > 1) The embedded vec has a trailing "flexible" array member with > > its > >
[PING][PATCH] define auto_vec copy ctor and assignment (PR 90904)
Ping: https://gcc.gnu.org/pipermail/gcc-patches/2021-June/573968.html Any questions/suggestions on the final patch or is it okay to commit? On 6/29/21 7:46 PM, Martin Sebor wrote: On 6/29/21 4:58 AM, Richard Biener wrote: On Mon, Jun 28, 2021 at 8:07 PM Martin Sebor wrote: On 6/28/21 2:07 AM, Richard Biener wrote: On Sat, Jun 26, 2021 at 12:36 AM Martin Sebor wrote: On 6/25/21 4:11 PM, Jason Merrill wrote: On 6/25/21 4:51 PM, Martin Sebor wrote: On 6/1/21 3:38 PM, Jason Merrill wrote: On 6/1/21 3:56 PM, Martin Sebor wrote: On 5/27/21 2:53 PM, Jason Merrill wrote: On 4/27/21 11:52 AM, Martin Sebor via Gcc-patches wrote: On 4/27/21 8:04 AM, Richard Biener wrote: On Tue, Apr 27, 2021 at 3:59 PM Martin Sebor wrote: On 4/27/21 1:58 AM, Richard Biener wrote: On Tue, Apr 27, 2021 at 2:46 AM Martin Sebor via Gcc-patches wrote: PR 90904 notes that auto_vec is unsafe to copy and assign because the class manages its own memory but doesn't define (or delete) either special function. Since I first ran into the problem, auto_vec has grown a move ctor and move assignment from a dynamically-allocated vec but still no copy ctor or copy assignment operator. The attached patch adds the two special functions to auto_vec along with a few simple tests. It makes auto_vec safe to use in containers that expect copyable and assignable element types and passes bootstrap and regression testing on x86_64-linux. The question is whether we want such uses to appear since those can be quite inefficient? Thus the option is to delete those operators? I would strongly prefer the generic vector class to have the properties expected of any other generic container: copyable and assignable. If we also want another vector type with this restriction I suggest to add another "noncopyable" type and make that property explicit in its name. I can submit one in a followup patch if you think we need one. I'm not sure (and not strictly against the copy and assign). Looking around I see that vec<> does not do deep copying. Making auto_vec<> do it might be surprising (I added the move capability to match how vec<> is used - as "reference" to a vector) The vec base classes are special: they have no ctors at all (because of their use in unions). That's something we might have to live with but it's not a model to follow in ordinary containers. I don't think we have to live with it anymore, now that we're writing C++11. The auto_vec class was introduced to fill the need for a conventional sequence container with a ctor and dtor. The missing copy ctor and assignment operators were an oversight, not a deliberate feature. This change fixes that oversight. The revised patch also adds a copy ctor/assignment to the auto_vec primary template (that's also missing it). In addition, it adds a new class called auto_vec_ncopy that disables copying and assignment as you prefer. Hmm, adding another class doesn't really help with the confusion richi mentions. And many uses of auto_vec will pass them as vec, which will still do a shallow copy. I think it's probably better to disable the copy special members for auto_vec until we fix vec<>. There are at least a couple of problems that get in the way of fixing all of vec to act like a well-behaved C++ container: 1) The embedded vec has a trailing "flexible" array member with its instances having different size. They're initialized by memset and copied by memcpy. The class can't have copy ctors or assignments but it should disable/delete them instead. 2) The heap-based vec is used throughout GCC with the assumption of shallow copy semantics (not just as function arguments but also as members of other such POD classes). This can be changed by providing copy and move ctors and assignment operators for it, and also for some of the classes in which it's a member and that are used with the same assumption. 3) The heap-based vec::block_remove() assumes its elements are PODs. That breaks in VEC_ORDERED_REMOVE_IF (used in gcc/dwarf2cfi.c:2862 and tree-vect-patterns.c). I took a stab at both and while (1) is easy, (2) is shaping up to be a big and tricky project. Tricky because it involves using std::move in places where what's moved is subsequently still used. I can keep plugging away at it but it won't change the fact that the embedded and heap-based vecs have different requirements. It doesn't seem to me that having a safely copyable auto_vec needs to be put on hold until the rats nest above is untangled. It won't make anything worse than it is. (I have a project that depends on a sane auto_vec working). A couple of alternatives to solving this are to use std::vector or write an equivalent vector class just for GCC. It occurs to me that another way to work around the issue of passing an auto_vec by value as a vec, and thus doing a shallow copy, would be to add a vec ctor taking an auto_vec, and delete that. This would mean if
[PING][PATCH] define auto_vec copy ctor and assignment (PR 90904)
Ping: https://gcc.gnu.org/pipermail/gcc-patches/2021-April/568901.html On 4/27/21 9:52 AM, Martin Sebor wrote: On 4/27/21 8:04 AM, Richard Biener wrote: On Tue, Apr 27, 2021 at 3:59 PM Martin Sebor wrote: On 4/27/21 1:58 AM, Richard Biener wrote: On Tue, Apr 27, 2021 at 2:46 AM Martin Sebor via Gcc-patches wrote: PR 90904 notes that auto_vec is unsafe to copy and assign because the class manages its own memory but doesn't define (or delete) either special function. Since I first ran into the problem, auto_vec has grown a move ctor and move assignment from a dynamically-allocated vec but still no copy ctor or copy assignment operator. The attached patch adds the two special functions to auto_vec along with a few simple tests. It makes auto_vec safe to use in containers that expect copyable and assignable element types and passes bootstrap and regression testing on x86_64-linux. The question is whether we want such uses to appear since those can be quite inefficient? Thus the option is to delete those operators? I would strongly prefer the generic vector class to have the properties expected of any other generic container: copyable and assignable. If we also want another vector type with this restriction I suggest to add another "noncopyable" type and make that property explicit in its name. I can submit one in a followup patch if you think we need one. I'm not sure (and not strictly against the copy and assign). Looking around I see that vec<> does not do deep copying. Making auto_vec<> do it might be surprising (I added the move capability to match how vec<> is used - as "reference" to a vector) The vec base classes are special: they have no ctors at all (because of their use in unions). That's something we might have to live with but it's not a model to follow in ordinary containers. The auto_vec class was introduced to fill the need for a conventional sequence container with a ctor and dtor. The missing copy ctor and assignment operators were an oversight, not a deliberate feature. This change fixes that oversight. The revised patch also adds a copy ctor/assignment to the auto_vec primary template (that's also missing it). In addition, it adds a new class called auto_vec_ncopy that disables copying and assignment as you prefer. It also disables copying for the auto_string_vec class. Martin
