Re: [PATCH v2] c++: Support target-specific nodes with streaming [PR98645,PR111224]
On 3/12/24 08:21, Nathaniel Shead wrote:
On Tue, Mar 12, 2024 at 11:11:40PM +1100, Nathaniel Shead wrote:
On Mon, Mar 11, 2024 at 10:36:06AM -0400, Patrick Palka wrote:
On Sun, 10 Mar 2024, Nathaniel Shead wrote:
Bootstrapped and regtested on x86_64-pc-linux-gnu and
aarch64-unknown-linux-gnu, OK for trunk?
It's worth noting that the AArch64 machines I had available to test with
didn't have a new enough glibc to reproduce the ICEs in the PR, but this
patch will be necessary (albeit possibly not sufficient) to fix it.
-- >8 --
Some targets make use of POLY_INT_CSTs and other custom builtin types,
which currently violate some assumptions when streaming. This patch adds
support for them, specifically AArch64 SVE types like __fp16.
It seems other built-in types are handled by adding them to the
fixed_trees vector in init_modules (and then we install them first
during streaming). Could we just add all the target-specific types to
fixed_trees too?
Yes, that works too. Seems cleaner as well, though I had to add it as a
separate loop because the set of builtin types registered is not
determined until runtiem (depending on e.g. ABI flags). I also noticed
that this fixes another PR, on PowerPC, so I've added a test for it.
Thanks!
Bootstrapped and regtested on x86_64-pc-linux-gnu,
aarch64-unknown-linux-gnu, and powerpc64le-unknown-linux-gnu;
OK for trunk?
-- >8 --
Some targets make use of POLY_INT_CSTs and other custom builtin types,
which currently violate some assumptions when streaming. This patch adds
support for them, such as types like Aarch64 __fp16, PowerPC __ibm128,
and vector types thereof.
This patch doesn't provide "full" support of AArch64 SVE, however, since
for that we would need to support 'target' nodes (tracked in PR108080).
Adding the new builtin types means that on Aarch64 we now have 217
global trees created on initialisation (up from 191), so this patch also
slightly bumps the initial size of the fixed_trees allocation to 250.
PR c++/98645
PR c++/111224
gcc/cp/ChangeLog:
* module.cc (enum tree_tag): Add new tag for builtin types.
(trees_out::start): POLY_INT_CSTs can be emitted.
(trees_in::start): Likewise.
(trees_out::core_vals): Stream POLY_INT_CSTs.
(trees_in::core_vals): Likewise.
(trees_out::type_node): Handle vectors with multiple coeffs.
(trees_in::tree_node): Likewise.
(init_modules): Register target-specific builtin types. Bump
initial capacity slightly.
gcc/testsuite/ChangeLog:
* g++.dg/modules/target-aarch64-1_a.C: New test.
* g++.dg/modules/target-aarch64-1_b.C: New test.
* g++.dg/modules/target-powerpc-1_a.C: New test.
* g++.dg/modules/target-powerpc-1_b.C: New test.
Signed-off-by: Nathaniel Shead
Reviewed-by: Patrick Palka
---
gcc/cp/module.cc | 32 +--
.../g++.dg/modules/target-aarch64-1_a.C | 17 ++
.../g++.dg/modules/target-aarch64-1_b.C | 13
.../g++.dg/modules/target-powerpc-1_a.C | 7
.../g++.dg/modules/target-powerpc-1_b.C | 10 ++
5 files changed, 69 insertions(+), 10 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/modules/target-aarch64-1_a.C
create mode 100644 gcc/testsuite/g++.dg/modules/target-aarch64-1_b.C
create mode 100644 gcc/testsuite/g++.dg/modules/target-powerpc-1_a.C
create mode 100644 gcc/testsuite/g++.dg/modules/target-powerpc-1_b.C
diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
index 99055523d91..8aab9ea0bae 100644
--- a/gcc/cp/module.cc
+++ b/gcc/cp/module.cc
@@ -5173,7 +5173,6 @@ trees_out::start (tree t, bool code_streamed)
break;
case FIXED_CST:
-case POLY_INT_CST:
gcc_unreachable (); /* Not supported in C++. */
break;
@@ -5259,7 +5258,6 @@ trees_in::start (unsigned code)
case FIXED_CST:
case IDENTIFIER_NODE:
-case POLY_INT_CST:
case SSA_NAME:
case TARGET_MEM_REF:
case TRANSLATION_UNIT_DECL:
@@ -6106,7 +6104,10 @@ trees_out::core_vals (tree t)
break;
case POLY_INT_CST:
- gcc_unreachable (); /* Not supported in C++. */
+ if (streaming_p ())
+ for (unsigned ix = 0; ix != NUM_POLY_INT_COEFFS; ix++)
+ WT (POLY_INT_CST_COEFF (t, ix));
+ break;
case REAL_CST:
if (streaming_p ())
@@ -6615,8 +6616,9 @@ trees_in::core_vals (tree t)
break;
case POLY_INT_CST:
- /* Not suported in C++. */
- return false;
+ for (unsigned ix = 0; ix != NUM_POLY_INT_COEFFS; ix++)
+ RT (POLY_INT_CST_COEFF (t, ix));
+ break;
case REAL_CST:
if (const void *bytes = buf (sizeof (real_value)))
@@ -9068,8 +9070,8 @@ trees_out::type_node (tree type)
if (streaming_p ())
{
poly_uint64 nunits = TYPE_VECTOR_SUBPARTS (type);
- /* to_constant asserts that only coeff[0] is of interest. */
-
Re: [PATCH v2] c++: Support target-specific nodes with streaming [PR98645,PR111224]
On Tue, 12 Mar 2024, Nathaniel Shead wrote: > On Tue, Mar 12, 2024 at 11:11:40PM +1100, Nathaniel Shead wrote: > > On Mon, Mar 11, 2024 at 10:36:06AM -0400, Patrick Palka wrote: > > > On Sun, 10 Mar 2024, Nathaniel Shead wrote: > > > > > > > Bootstrapped and regtested on x86_64-pc-linux-gnu and > > > > aarch64-unknown-linux-gnu, OK for trunk? > > > > > > > > It's worth noting that the AArch64 machines I had available to test with > > > > didn't have a new enough glibc to reproduce the ICEs in the PR, but this > > > > patch will be necessary (albeit possibly not sufficient) to fix it. > > > > > > > > -- >8 -- > > > > > > > > Some targets make use of POLY_INT_CSTs and other custom builtin types, > > > > which currently violate some assumptions when streaming. This patch adds > > > > support for them, specifically AArch64 SVE types like __fp16. > > > > > > It seems other built-in types are handled by adding them to the > > > fixed_trees vector in init_modules (and then we install them first > > > during streaming). Could we just add all the target-specific types to > > > fixed_trees too? > > > > > > > Yes, that works too. Seems cleaner as well, though I had to add it as a > > separate loop because the set of builtin types registered is not > > determined until runtiem (depending on e.g. ABI flags). I also noticed > > that this fixes another PR, on PowerPC, so I've added a test for it. > > Thanks! > > > > Bootstrapped and regtested on x86_64-pc-linux-gnu, > > aarch64-unknown-linux-gnu, and powerpc64le-unknown-linux-gnu; > > OK for trunk? > > > > -- >8 -- > > > > Some targets make use of POLY_INT_CSTs and other custom builtin types, > > which currently violate some assumptions when streaming. This patch adds > > support for them, such as types like Aarch64 __fp16, PowerPC __ibm128, > > and vector types thereof. > > > > This patch doesn't provide "full" support of AArch64 SVE, however, since > > for that we would need to support 'target' nodes (tracked in PR108080). > > > > Adding the new builtin types means that on Aarch64 we now have 217 > > global trees created on initialisation (up from 191), so this patch also > > slightly bumps the initial size of the fixed_trees allocation to 250. > > > > PR c++/98645 > > PR c++/111224 > > > > gcc/cp/ChangeLog: > > > > * module.cc (enum tree_tag): Add new tag for builtin types. > > (trees_out::start): POLY_INT_CSTs can be emitted. > > (trees_in::start): Likewise. > > (trees_out::core_vals): Stream POLY_INT_CSTs. > > (trees_in::core_vals): Likewise. > > (trees_out::type_node): Handle vectors with multiple coeffs. > > (trees_in::tree_node): Likewise. > > (init_modules): Register target-specific builtin types. Bump > > initial capacity slightly. > > > > gcc/testsuite/ChangeLog: > > > > * g++.dg/modules/target-aarch64-1_a.C: New test. > > * g++.dg/modules/target-aarch64-1_b.C: New test. > > * g++.dg/modules/target-powerpc-1_a.C: New test. > > * g++.dg/modules/target-powerpc-1_b.C: New test. > > > > Signed-off-by: Nathaniel Shead > > Reviewed-by: Patrick Palka > > --- > > gcc/cp/module.cc | 32 +-- > > .../g++.dg/modules/target-aarch64-1_a.C | 17 ++ > > .../g++.dg/modules/target-aarch64-1_b.C | 13 > > .../g++.dg/modules/target-powerpc-1_a.C | 7 > > .../g++.dg/modules/target-powerpc-1_b.C | 10 ++ > > 5 files changed, 69 insertions(+), 10 deletions(-) > > create mode 100644 gcc/testsuite/g++.dg/modules/target-aarch64-1_a.C > > create mode 100644 gcc/testsuite/g++.dg/modules/target-aarch64-1_b.C > > create mode 100644 gcc/testsuite/g++.dg/modules/target-powerpc-1_a.C > > create mode 100644 gcc/testsuite/g++.dg/modules/target-powerpc-1_b.C > > > > diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc > > index 99055523d91..8aab9ea0bae 100644 > > --- a/gcc/cp/module.cc > > +++ b/gcc/cp/module.cc > > @@ -5173,7 +5173,6 @@ trees_out::start (tree t, bool code_streamed) > >break; > > > > case FIXED_CST: > > -case POLY_INT_CST: > >gcc_unreachable (); /* Not supported in C++. */ > >break; > > > > @@ -5259,7 +5258,6 @@ trees_in::start (unsigned code) > > > > case FIXED_CST: > > case IDENTIFIER_NODE: > > -case POLY_INT_CST: > > case SSA_NAME: > > case TARGET_MEM_REF: > > case TRANSLATION_UNIT_DECL: > > @@ -6106,7 +6104,10 @@ trees_out::core_vals (tree t) > >break; > > > > case POLY_INT_CST: > > - gcc_unreachable (); /* Not supported in C++. */ > > + if (streaming_p ()) > > + for (unsigned ix = 0; ix != NUM_POLY_INT_COEFFS; ix++) > > + WT (POLY_INT_CST_COEFF (t, ix)); > > + break; > > > > case REAL_CST: > >if (streaming_p ()) > > @@ -6615,8 +6616,9 @@ trees_in::core_vals (tree t) > >break; > > > > case POLY_INT_CST: > > - /* Not suported in C++. */ > > - return
Re: [PATCH v2] c++: Support target-specific nodes with streaming [PR98645,PR111224]
On Tue, Mar 12, 2024 at 11:11:40PM +1100, Nathaniel Shead wrote: > On Mon, Mar 11, 2024 at 10:36:06AM -0400, Patrick Palka wrote: > > On Sun, 10 Mar 2024, Nathaniel Shead wrote: > > > > > Bootstrapped and regtested on x86_64-pc-linux-gnu and > > > aarch64-unknown-linux-gnu, OK for trunk? > > > > > > It's worth noting that the AArch64 machines I had available to test with > > > didn't have a new enough glibc to reproduce the ICEs in the PR, but this > > > patch will be necessary (albeit possibly not sufficient) to fix it. > > > > > > -- >8 -- > > > > > > Some targets make use of POLY_INT_CSTs and other custom builtin types, > > > which currently violate some assumptions when streaming. This patch adds > > > support for them, specifically AArch64 SVE types like __fp16. > > > > It seems other built-in types are handled by adding them to the > > fixed_trees vector in init_modules (and then we install them first > > during streaming). Could we just add all the target-specific types to > > fixed_trees too? > > > > Yes, that works too. Seems cleaner as well, though I had to add it as a > separate loop because the set of builtin types registered is not > determined until runtiem (depending on e.g. ABI flags). I also noticed > that this fixes another PR, on PowerPC, so I've added a test for it. > Thanks! > > Bootstrapped and regtested on x86_64-pc-linux-gnu, > aarch64-unknown-linux-gnu, and powerpc64le-unknown-linux-gnu; > OK for trunk? > > -- >8 -- > > Some targets make use of POLY_INT_CSTs and other custom builtin types, > which currently violate some assumptions when streaming. This patch adds > support for them, such as types like Aarch64 __fp16, PowerPC __ibm128, > and vector types thereof. > > This patch doesn't provide "full" support of AArch64 SVE, however, since > for that we would need to support 'target' nodes (tracked in PR108080). > > Adding the new builtin types means that on Aarch64 we now have 217 > global trees created on initialisation (up from 191), so this patch also > slightly bumps the initial size of the fixed_trees allocation to 250. > > PR c++/98645 > PR c++/111224 > > gcc/cp/ChangeLog: > > * module.cc (enum tree_tag): Add new tag for builtin types. > (trees_out::start): POLY_INT_CSTs can be emitted. > (trees_in::start): Likewise. > (trees_out::core_vals): Stream POLY_INT_CSTs. > (trees_in::core_vals): Likewise. > (trees_out::type_node): Handle vectors with multiple coeffs. > (trees_in::tree_node): Likewise. > (init_modules): Register target-specific builtin types. Bump > initial capacity slightly. > > gcc/testsuite/ChangeLog: > > * g++.dg/modules/target-aarch64-1_a.C: New test. > * g++.dg/modules/target-aarch64-1_b.C: New test. > * g++.dg/modules/target-powerpc-1_a.C: New test. > * g++.dg/modules/target-powerpc-1_b.C: New test. > > Signed-off-by: Nathaniel Shead > Reviewed-by: Patrick Palka > --- > gcc/cp/module.cc | 32 +-- > .../g++.dg/modules/target-aarch64-1_a.C | 17 ++ > .../g++.dg/modules/target-aarch64-1_b.C | 13 > .../g++.dg/modules/target-powerpc-1_a.C | 7 > .../g++.dg/modules/target-powerpc-1_b.C | 10 ++ > 5 files changed, 69 insertions(+), 10 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/modules/target-aarch64-1_a.C > create mode 100644 gcc/testsuite/g++.dg/modules/target-aarch64-1_b.C > create mode 100644 gcc/testsuite/g++.dg/modules/target-powerpc-1_a.C > create mode 100644 gcc/testsuite/g++.dg/modules/target-powerpc-1_b.C > > diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc > index 99055523d91..8aab9ea0bae 100644 > --- a/gcc/cp/module.cc > +++ b/gcc/cp/module.cc > @@ -5173,7 +5173,6 @@ trees_out::start (tree t, bool code_streamed) >break; > > case FIXED_CST: > -case POLY_INT_CST: >gcc_unreachable (); /* Not supported in C++. */ >break; > > @@ -5259,7 +5258,6 @@ trees_in::start (unsigned code) > > case FIXED_CST: > case IDENTIFIER_NODE: > -case POLY_INT_CST: > case SSA_NAME: > case TARGET_MEM_REF: > case TRANSLATION_UNIT_DECL: > @@ -6106,7 +6104,10 @@ trees_out::core_vals (tree t) >break; > > case POLY_INT_CST: > - gcc_unreachable (); /* Not supported in C++. */ > + if (streaming_p ()) > + for (unsigned ix = 0; ix != NUM_POLY_INT_COEFFS; ix++) > + WT (POLY_INT_CST_COEFF (t, ix)); > + break; > > case REAL_CST: >if (streaming_p ()) > @@ -6615,8 +6616,9 @@ trees_in::core_vals (tree t) >break; > > case POLY_INT_CST: > - /* Not suported in C++. */ > - return false; > + for (unsigned ix = 0; ix != NUM_POLY_INT_COEFFS; ix++) > + RT (POLY_INT_CST_COEFF (t, ix)); > + break; > > case REAL_CST: >if (const void *bytes = buf (sizeof (real_value))) > @@ -9068,8 +9070,8 @@
[PATCH v2] c++: Support target-specific nodes with streaming [PR98645,PR111224]
On Mon, Mar 11, 2024 at 10:36:06AM -0400, Patrick Palka wrote:
> On Sun, 10 Mar 2024, Nathaniel Shead wrote:
>
> > Bootstrapped and regtested on x86_64-pc-linux-gnu and
> > aarch64-unknown-linux-gnu, OK for trunk?
> >
> > It's worth noting that the AArch64 machines I had available to test with
> > didn't have a new enough glibc to reproduce the ICEs in the PR, but this
> > patch will be necessary (albeit possibly not sufficient) to fix it.
> >
> > -- >8 --
> >
> > Some targets make use of POLY_INT_CSTs and other custom builtin types,
> > which currently violate some assumptions when streaming. This patch adds
> > support for them, specifically AArch64 SVE types like __fp16.
>
> It seems other built-in types are handled by adding them to the
> fixed_trees vector in init_modules (and then we install them first
> during streaming). Could we just add all the target-specific types to
> fixed_trees too?
>
Yes, that works too. Seems cleaner as well, though I had to add it as a
separate loop because the set of builtin types registered is not
determined until runtiem (depending on e.g. ABI flags). I also noticed
that this fixes another PR, on PowerPC, so I've added a test for it.
Thanks!
Bootstrapped and regtested on x86_64-pc-linux-gnu,
aarch64-unknown-linux-gnu, and powerpc64le-unknown-linux-gnu;
OK for trunk?
-- >8 --
Some targets make use of POLY_INT_CSTs and other custom builtin types,
which currently violate some assumptions when streaming. This patch adds
support for them, such as types like Aarch64 __fp16, PowerPC __ibm128,
and vector types thereof.
This patch doesn't provide "full" support of AArch64 SVE, however, since
for that we would need to support 'target' nodes (tracked in PR108080).
Adding the new builtin types means that on Aarch64 we now have 217
global trees created on initialisation (up from 191), so this patch also
slightly bumps the initial size of the fixed_trees allocation to 250.
PR c++/98645
PR c++/111224
gcc/cp/ChangeLog:
* module.cc (enum tree_tag): Add new tag for builtin types.
(trees_out::start): POLY_INT_CSTs can be emitted.
(trees_in::start): Likewise.
(trees_out::core_vals): Stream POLY_INT_CSTs.
(trees_in::core_vals): Likewise.
(trees_out::type_node): Handle vectors with multiple coeffs.
(trees_in::tree_node): Likewise.
(init_modules): Register target-specific builtin types. Bump
initial capacity slightly.
gcc/testsuite/ChangeLog:
* g++.dg/modules/target-aarch64-1_a.C: New test.
* g++.dg/modules/target-aarch64-1_b.C: New test.
* g++.dg/modules/target-powerpc-1_a.C: New test.
* g++.dg/modules/target-powerpc-1_b.C: New test.
Signed-off-by: Nathaniel Shead
Reviewed-by: Patrick Palka
---
gcc/cp/module.cc | 32 +--
.../g++.dg/modules/target-aarch64-1_a.C | 17 ++
.../g++.dg/modules/target-aarch64-1_b.C | 13
.../g++.dg/modules/target-powerpc-1_a.C | 7
.../g++.dg/modules/target-powerpc-1_b.C | 10 ++
5 files changed, 69 insertions(+), 10 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/modules/target-aarch64-1_a.C
create mode 100644 gcc/testsuite/g++.dg/modules/target-aarch64-1_b.C
create mode 100644 gcc/testsuite/g++.dg/modules/target-powerpc-1_a.C
create mode 100644 gcc/testsuite/g++.dg/modules/target-powerpc-1_b.C
diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
index 99055523d91..8aab9ea0bae 100644
--- a/gcc/cp/module.cc
+++ b/gcc/cp/module.cc
@@ -5173,7 +5173,6 @@ trees_out::start (tree t, bool code_streamed)
break;
case FIXED_CST:
-case POLY_INT_CST:
gcc_unreachable (); /* Not supported in C++. */
break;
@@ -5259,7 +5258,6 @@ trees_in::start (unsigned code)
case FIXED_CST:
case IDENTIFIER_NODE:
-case POLY_INT_CST:
case SSA_NAME:
case TARGET_MEM_REF:
case TRANSLATION_UNIT_DECL:
@@ -6106,7 +6104,10 @@ trees_out::core_vals (tree t)
break;
case POLY_INT_CST:
- gcc_unreachable (); /* Not supported in C++. */
+ if (streaming_p ())
+ for (unsigned ix = 0; ix != NUM_POLY_INT_COEFFS; ix++)
+ WT (POLY_INT_CST_COEFF (t, ix));
+ break;
case REAL_CST:
if (streaming_p ())
@@ -6615,8 +6616,9 @@ trees_in::core_vals (tree t)
break;
case POLY_INT_CST:
- /* Not suported in C++. */
- return false;
+ for (unsigned ix = 0; ix != NUM_POLY_INT_COEFFS; ix++)
+ RT (POLY_INT_CST_COEFF (t, ix));
+ break;
case REAL_CST:
if (const void *bytes = buf (sizeof (real_value)))
@@ -9068,8 +9070,8 @@ trees_out::type_node (tree type)
if (streaming_p ())
{
poly_uint64 nunits = TYPE_VECTOR_SUBPARTS (type);
- /* to_constant asserts that only coeff[0] is of interest. */
- wu (static_cast (nunits.to_constant ()));
+ for (unsigned ix = 0; ix !=
