Re: [PATCH] c++/modules: Support target-specific nodes with streaming [PR111224]

[Patrick Palka <ppalka@redhat.com>]

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?

> 
> This patch doesn't provide "full" support of AArch64 SVE, however, since
> for that we would need to support 'target' nodes (tracked in PR108080).
> 
> 	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 target-specific builtin types,
> 	and vectors with NUM_POLY_INT_COEFFS > 1.
> 	(trees_in::tree_node): Likewise.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/modules/pr111224_a.C: New test.
> 	* g++.dg/modules/pr111224_b.C: New test.
> 
> Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
> ---
>  gcc/cp/module.cc                          | 70 +++++++++++++++++++----
>  gcc/testsuite/g++.dg/modules/pr111224_a.C | 17 ++++++
>  gcc/testsuite/g++.dg/modules/pr111224_b.C | 13 +++++
>  3 files changed, 90 insertions(+), 10 deletions(-)
>  create mode 100644 gcc/testsuite/g++.dg/modules/pr111224_a.C
>  create mode 100644 gcc/testsuite/g++.dg/modules/pr111224_b.C
> 
> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> index 99055523d91..0b5e2e67053 100644
> --- a/gcc/cp/module.cc
> +++ b/gcc/cp/module.cc
> @@ -2718,6 +2718,7 @@ enum tree_tag {
>    tt_typedef_type,	/* A (possibly implicit) typedefed type.  */
>    tt_derived_type,	/* A type derived from another type.  */
>    tt_variant_type,	/* A variant of another type.  */
> +  tt_builtin_type,      /* A custom builtin type.  */
>  
>    tt_tinfo_var,		/* Typeinfo object. */
>    tt_tinfo_typedef,	/* Typeinfo typedef.  */
> @@ -2732,7 +2733,7 @@ enum tree_tag {
>    tt_binfo,		/* A BINFO.  */
>    tt_vtable,		/* A vtable.  */
>    tt_thunk,		/* A thunk.  */
> -  tt_clone_ref,
> +  tt_clone_ref,         /* A cloned function.  */
>  
>    tt_entity,		/* A extra-cluster entity.  */
>  
> @@ -5173,7 +5174,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 +5259,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 +6105,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 +6617,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)))
> @@ -8930,6 +8933,32 @@ trees_out::type_node (tree type)
>        return;
>      }
>  
> +  if (tree name = TYPE_NAME (type))
> +    if (TREE_CODE (name) == TYPE_DECL && DECL_ARTIFICIAL (name))
> +      {
> +	/* Potentially a custom machine- or OS-specific builtin type.  */
> +	bool found = false;
> +	unsigned ix = 0;
> +	for (tree t = registered_builtin_types; t; t = TREE_CHAIN (t), ix++)
> +	  if (TREE_VALUE (t) == type)
> +	    {
> +	      found = true;
> +	      break;
> +	    }
> +	if (found)
> +	  {
> +	    int type_tag = insert (type);
> +	    if (streaming_p ())
> +	      {
> +		i (tt_builtin_type);
> +		u (ix);
> +		dump (dumper::TREE)
> +		  && dump ("Wrote:%d builtin type %N", type_tag, name);
> +	      }
> +	    return;
> +	  }
> +      }
> +
>    if (streaming_p ())
>      {
>        u (tt_derived_type);
> @@ -9068,8 +9097,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<unsigned HOST_WIDE_INT> (nunits.to_constant ()));
> +	  for (unsigned ix = 0; ix != NUM_POLY_INT_COEFFS; ix++)
> +	    wu (nunits.coeffs[ix]);
>  	}
>        break;
>      }
> @@ -9630,9 +9659,11 @@ trees_in::tree_node (bool is_use)
>  
>  	  case VECTOR_TYPE:
>  	    {
> -	      unsigned HOST_WIDE_INT nunits = wu ();
> +	      poly_uint64 nunits;
> +	      for (unsigned ix = 0; ix != NUM_POLY_INT_COEFFS; ix++)
> +		nunits.coeffs[ix] = wu ();
>  	      if (!get_overrun ())
> -		res = build_vector_type (res, static_cast<poly_int64> (nunits));
> +		res = build_vector_type (res, nunits);
>  	    }
>  	    break;
>  	  }
> @@ -9700,6 +9731,25 @@ trees_in::tree_node (bool is_use)
>        }
>        break;
>  
> +    case tt_builtin_type:
> +      /* A machine- or OS-specific builtin type.  */
> +      {
> +	unsigned ix = u ();
> +	res = registered_builtin_types;
> +	for (; ix && res; res = TREE_CHAIN (res))
> +	  ix--;
> +	if (!res)
> +	  set_overrun ();
> +	if (!get_overrun ())
> +	  {
> +	    res = TREE_VALUE (res);
> +	    int type_tag = insert (res);
> +	    dump (dumper::TREE)
> +	      && dump ("Read:%d builtin type %N", type_tag, res);
> +	  }
> +      }
> +      break;
> +
>      case tt_tinfo_var:
>      case tt_tinfo_typedef:
>        /* A tinfo var or typedef.  */
> diff --git a/gcc/testsuite/g++.dg/modules/pr111224_a.C b/gcc/testsuite/g++.dg/modules/pr111224_a.C
> new file mode 100644
> index 00000000000..6c699053cdc
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/pr111224_a.C
> @@ -0,0 +1,17 @@
> +// PR c++/111224
> +// { dg-do compile { target aarch64*-*-* } }
> +// { dg-require-effective-target aarch64_asm_sve_ok }
> +// { dg-additional-options "-fmodules-ts -march=armv8.2-a+sve" }
> +
> +module;
> +
> +// We can't do a header unit of this right now because this
> +// uses target attributes, that we don't yet support.
> +// See also PR c++/108080.
> +#include <arm_sve.h>
> +
> +export module M;
> +
> +export inline void foo(svbool_t x, svfloat16_t f) {
> +  svabs_f16_x(x, f);
> +}
> diff --git a/gcc/testsuite/g++.dg/modules/pr111224_b.C b/gcc/testsuite/g++.dg/modules/pr111224_b.C
> new file mode 100644
> index 00000000000..c18691dcf8a
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/pr111224_b.C
> @@ -0,0 +1,13 @@
> +// PR c++/111224
> +// { dg-module-do link { target aarch64*-*-* } }
> +// { dg-require-effective-target aarch64_asm_sve_ok }
> +// { dg-additional-options "-fmodules-ts -fno-module-lazy -march=armv8.2-a+sve" }
> +
> +#include <arm_sve.h>
> +import M;
> +
> +int main() {
> +  svbool_t x = svptrue_b8 ();
> +  svfloat16_t f = svdup_n_f16(1.0);
> +  foo(x, f);
> +}
> -- 
> 2.43.2
> 
>