[PATCH 0/4] c++/modules: Fix missed GMF discarding

[PATCH 0/4] c++/modules: Fix missed GMF discarding

[Nathaniel Shead]

This patch series fixes a number of issues with declarations in the GMF not
correctly getting discarded, and ending up in the module CMI.  The most
noticeable outcome of this is users running very quickly into issues with
including a header after those names have already been provided by an 'import';
this should go away once we implement deduplication of textual redefinitions,
but it's still worth discarding as it otherwise inflates the size of the CMI
with unused declarations.

Nathaniel Shead (4):
  c++: Don't emit unused GMF partial specializations [PR114630]
  c++: Track module purview for deferred instantiations [PR114630]
  c++: Also track module purview from deferred vtable instantiation
    [PR114630]
  c++: Add new xtreme-header testcase for GMF discarding

 gcc/cp/cp-tree.h                              |  3 +
 gcc/cp/decl2.cc                               | 11 ++-
 gcc/cp/module.cc                              | 75 ++++++++++++-------
 gcc/cp/pt.cc                                  |  4 +
 gcc/testsuite/g++.dg/modules/gmf-3.C          | 13 ++++
 gcc/testsuite/g++.dg/modules/gmf-4.C          | 27 +++++++
 gcc/testsuite/g++.dg/modules/partial-3.C      | 20 +++++
 .../g++.dg/modules/xtreme-header-8.C          |  9 +++
 gcc/testsuite/g++.dg/modules/xtreme-header.h  | 24 ++++--
 9 files changed, 147 insertions(+), 39 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/modules/gmf-3.C
 create mode 100644 gcc/testsuite/g++.dg/modules/gmf-4.C
 create mode 100644 gcc/testsuite/g++.dg/modules/partial-3.C
 create mode 100644 gcc/testsuite/g++.dg/modules/xtreme-header-8.C

-- 
2.43.2

[PATCH 1/4] c++/modules: Don't emit unused GMF partial specializations [PR114630]

[Nathaniel Shead]

Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?

-- >8 --

The change in r14-8408 to also emit partial specializations in the
global module fragment caused the regression in the linked PR; this
patch fixes this by restricting emitted GM partial specializations to
those that are actually used.

	PR c++/114630

gcc/cp/ChangeLog:

	* module.cc (depset::hash::add_partial_entities): Mark GM
	specializations as unreached.
	(depset::hash::find_dependencies): Also reach entities in the
	DECL_TEMPLATE_SPECIALIZATIONS list.

gcc/testsuite/ChangeLog:

	* g++.dg/modules/partial-3.C: New test.

Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
---
 gcc/cp/module.cc                         | 75 +++++++++++++++---------
 gcc/testsuite/g++.dg/modules/partial-3.C | 20 +++++++
 2 files changed, 66 insertions(+), 29 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/modules/partial-3.C

diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
index fac0301d80e..02b0ab3d687 100644
--- a/gcc/cp/module.cc
+++ b/gcc/cp/module.cc
@@ -13304,14 +13304,22 @@ depset::hash::add_partial_entities (vec<tree, va_gc> *partial_classes)
       depset *dep = make_dependency (inner, depset::EK_DECL);
 
       if (dep->get_entity_kind () == depset::EK_REDIRECT)
-	/* We should have recorded the template as a partial
-	   specialization.  */
-	gcc_checking_assert (dep->deps[0]->get_entity_kind ()
-			     == depset::EK_PARTIAL);
+	{
+	  dep = dep->deps[0];
+	  /* We should have recorded the template as a partial
+	     specialization.  */
+	  gcc_checking_assert (dep->get_entity_kind ()
+			       == depset::EK_PARTIAL);
+	}
       else
 	/* It was an explicit specialization, not a partial one.  */
 	gcc_checking_assert (dep->get_entity_kind ()
 			     == depset::EK_SPECIALIZATION);
+
+      /* Only emit GM entities if reached.  */
+      if (!DECL_LANG_SPECIFIC (inner)
+	  || !DECL_MODULE_PURVIEW_P (inner))
+	dep->set_flag_bit<DB_UNREACHED_BIT> ();
     }
 }
 
@@ -13632,31 +13640,40 @@ depset::hash::find_dependencies (module_state *module)
 	      if (!walker.is_key_order ()
 		  && TREE_CODE (decl) == TEMPLATE_DECL
 		  && !DECL_UNINSTANTIATED_TEMPLATE_FRIEND_P (decl))
-		/* Mark all the explicit & partial specializations as
-		   reachable.  */
-		for (tree cons = DECL_TEMPLATE_INSTANTIATIONS (decl);
-		     cons; cons = TREE_CHAIN (cons))
-		  {
-		    tree spec = TREE_VALUE (cons);
-		    if (TYPE_P (spec))
-		      spec = TYPE_NAME (spec);
-		    int use_tpl;
-		    node_template_info (spec, use_tpl);
-		    if (use_tpl & 2)
-		      {
-			depset *spec_dep = find_dependency (spec);
-			if (spec_dep->get_entity_kind () == EK_REDIRECT)
-			  spec_dep = spec_dep->deps[0];
-			if (spec_dep->is_unreached ())
-			  {
-			    reached_unreached = true;
-			    spec_dep->clear_flag_bit<DB_UNREACHED_BIT> ();
-			    dump (dumper::DEPEND)
-			      && dump ("Reaching unreached specialization"
-				       " %C:%N", TREE_CODE (spec), spec);
-			  }
-		      }
-		  }
+		{
+		  /* Mark all the explicit & partial specializations as
+		     reachable.  We search both specialization lists as some
+		     constrained partial specializations for class types are
+		     only found in DECL_TEMPLATE_SPECIALIZATIONS.  */
+		  auto mark_reached = [this](tree spec)
+		    {
+		      if (TYPE_P (spec))
+			spec = TYPE_NAME (spec);
+		      int use_tpl;
+		      node_template_info (spec, use_tpl);
+		      if (use_tpl & 2)
+			{
+			  depset *spec_dep = find_dependency (spec);
+			  if (spec_dep->get_entity_kind () == EK_REDIRECT)
+			    spec_dep = spec_dep->deps[0];
+			  if (spec_dep->is_unreached ())
+			    {
+			      reached_unreached = true;
+			      spec_dep->clear_flag_bit<DB_UNREACHED_BIT> ();
+			      dump (dumper::DEPEND)
+				&& dump ("Reaching unreached specialization"
+					 " %C:%N", TREE_CODE (spec), spec);
+			    }
+			}
+		    };
+
+		  for (tree cons = DECL_TEMPLATE_INSTANTIATIONS (decl);
+		       cons; cons = TREE_CHAIN (cons))
+		    mark_reached (TREE_VALUE (cons));
+		  for (tree cons = DECL_TEMPLATE_SPECIALIZATIONS (decl);
+		       cons; cons = TREE_CHAIN (cons))
+		    mark_reached (TREE_VALUE (cons));
+		}
 
 	      dump.outdent ();
 	      current = NULL;
diff --git a/gcc/testsuite/g++.dg/modules/partial-3.C b/gcc/testsuite/g++.dg/modules/partial-3.C
new file mode 100644
index 00000000000..0d498dad1bd
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/partial-3.C
@@ -0,0 +1,20 @@
+// PR c++/114630
+// { dg-additional-options "-fmodules-ts -std=c++20 -Wno-global-module -fdump-lang-module" }
+// { dg-module-cmi M }
+
+module;
+
+template <typename T> struct S {};
+
+template <typename T> struct S<T*> {};
+template <typename T> requires (sizeof(T) == 4) struct S<T*> {};
+
+template <typename T> int V = 0;
+
+template <typename T> int V<T*> = 1;
+template <typename T> requires (sizeof(T) == 4) int V<T*> = 2;
+
+export module M;
+
+// The whole GMF should be discarded here
+// { dg-final { scan-lang-dump "Wrote 0 clusters" module } }
-- 
2.43.2

Re: [PATCH 1/4] c++/modules: Don't emit unused GMF partial specializations [PR114630]

[Jason Merrill]

On 5/1/24 02:59, Nathaniel Shead wrote:
> Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?

OK.

> -- >8 --
> 
> The change in r14-8408 to also emit partial specializations in the
> global module fragment caused the regression in the linked PR; this
> patch fixes this by restricting emitted GM partial specializations to
> those that are actually used.
> 
> 	PR c++/114630
> 
> gcc/cp/ChangeLog:
> 
> 	* module.cc (depset::hash::add_partial_entities): Mark GM
> 	specializations as unreached.
> 	(depset::hash::find_dependencies): Also reach entities in the
> 	DECL_TEMPLATE_SPECIALIZATIONS list.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/modules/partial-3.C: New test.
> 
> Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
> ---
>   gcc/cp/module.cc                         | 75 +++++++++++++++---------
>   gcc/testsuite/g++.dg/modules/partial-3.C | 20 +++++++
>   2 files changed, 66 insertions(+), 29 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/modules/partial-3.C
> 
> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> index fac0301d80e..02b0ab3d687 100644
> --- a/gcc/cp/module.cc
> +++ b/gcc/cp/module.cc
> @@ -13304,14 +13304,22 @@ depset::hash::add_partial_entities (vec<tree, va_gc> *partial_classes)
>         depset *dep = make_dependency (inner, depset::EK_DECL);
>   
>         if (dep->get_entity_kind () == depset::EK_REDIRECT)
> -	/* We should have recorded the template as a partial
> -	   specialization.  */
> -	gcc_checking_assert (dep->deps[0]->get_entity_kind ()
> -			     == depset::EK_PARTIAL);
> +	{
> +	  dep = dep->deps[0];
> +	  /* We should have recorded the template as a partial
> +	     specialization.  */
> +	  gcc_checking_assert (dep->get_entity_kind ()
> +			       == depset::EK_PARTIAL);
> +	}
>         else
>   	/* It was an explicit specialization, not a partial one.  */
>   	gcc_checking_assert (dep->get_entity_kind ()
>   			     == depset::EK_SPECIALIZATION);
> +
> +      /* Only emit GM entities if reached.  */
> +      if (!DECL_LANG_SPECIFIC (inner)
> +	  || !DECL_MODULE_PURVIEW_P (inner))
> +	dep->set_flag_bit<DB_UNREACHED_BIT> ();
>       }
>   }
>   
> @@ -13632,31 +13640,40 @@ depset::hash::find_dependencies (module_state *module)
>   	      if (!walker.is_key_order ()
>   		  && TREE_CODE (decl) == TEMPLATE_DECL
>   		  && !DECL_UNINSTANTIATED_TEMPLATE_FRIEND_P (decl))
> -		/* Mark all the explicit & partial specializations as
> -		   reachable.  */
> -		for (tree cons = DECL_TEMPLATE_INSTANTIATIONS (decl);
> -		     cons; cons = TREE_CHAIN (cons))
> -		  {
> -		    tree spec = TREE_VALUE (cons);
> -		    if (TYPE_P (spec))
> -		      spec = TYPE_NAME (spec);
> -		    int use_tpl;
> -		    node_template_info (spec, use_tpl);
> -		    if (use_tpl & 2)
> -		      {
> -			depset *spec_dep = find_dependency (spec);
> -			if (spec_dep->get_entity_kind () == EK_REDIRECT)
> -			  spec_dep = spec_dep->deps[0];
> -			if (spec_dep->is_unreached ())
> -			  {
> -			    reached_unreached = true;
> -			    spec_dep->clear_flag_bit<DB_UNREACHED_BIT> ();
> -			    dump (dumper::DEPEND)
> -			      && dump ("Reaching unreached specialization"
> -				       " %C:%N", TREE_CODE (spec), spec);
> -			  }
> -		      }
> -		  }
> +		{
> +		  /* Mark all the explicit & partial specializations as
> +		     reachable.  We search both specialization lists as some
> +		     constrained partial specializations for class types are
> +		     only found in DECL_TEMPLATE_SPECIALIZATIONS.  */
> +		  auto mark_reached = [this](tree spec)
> +		    {
> +		      if (TYPE_P (spec))
> +			spec = TYPE_NAME (spec);
> +		      int use_tpl;
> +		      node_template_info (spec, use_tpl);
> +		      if (use_tpl & 2)
> +			{
> +			  depset *spec_dep = find_dependency (spec);
> +			  if (spec_dep->get_entity_kind () == EK_REDIRECT)
> +			    spec_dep = spec_dep->deps[0];
> +			  if (spec_dep->is_unreached ())
> +			    {
> +			      reached_unreached = true;
> +			      spec_dep->clear_flag_bit<DB_UNREACHED_BIT> ();
> +			      dump (dumper::DEPEND)
> +				&& dump ("Reaching unreached specialization"
> +					 " %C:%N", TREE_CODE (spec), spec);
> +			    }
> +			}
> +		    };
> +
> +		  for (tree cons = DECL_TEMPLATE_INSTANTIATIONS (decl);
> +		       cons; cons = TREE_CHAIN (cons))
> +		    mark_reached (TREE_VALUE (cons));
> +		  for (tree cons = DECL_TEMPLATE_SPECIALIZATIONS (decl);
> +		       cons; cons = TREE_CHAIN (cons))
> +		    mark_reached (TREE_VALUE (cons));
> +		}
>   
>   	      dump.outdent ();
>   	      current = NULL;
> diff --git a/gcc/testsuite/g++.dg/modules/partial-3.C b/gcc/testsuite/g++.dg/modules/partial-3.C
> new file mode 100644
> index 00000000000..0d498dad1bd
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/partial-3.C
> @@ -0,0 +1,20 @@
> +// PR c++/114630
> +// { dg-additional-options "-fmodules-ts -std=c++20 -Wno-global-module -fdump-lang-module" }
> +// { dg-module-cmi M }
> +
> +module;
> +
> +template <typename T> struct S {};
> +
> +template <typename T> struct S<T*> {};
> +template <typename T> requires (sizeof(T) == 4) struct S<T*> {};
> +
> +template <typename T> int V = 0;
> +
> +template <typename T> int V<T*> = 1;
> +template <typename T> requires (sizeof(T) == 4) int V<T*> = 2;
> +
> +export module M;
> +
> +// The whole GMF should be discarded here
> +// { dg-final { scan-lang-dump "Wrote 0 clusters" module } }

[PATCH 2/4] c++/modules: Track module purview for deferred instantiations [PR114630]

[Nathaniel Shead]

Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?

-- >8 --

When calling instantiate_pending_templates at end of parsing, any new
functions that are instantiated from this point have their module
purview set based on the current value of module_kind.

This is unideal, however, as the modules code will then treat these
instantiations as reachable and cause large swathes of the GMF to be
emitted into the module CMI, despite no code in the actual module
purview referencing it.

This patch fixes this by also remembering the value of module_kind when
the instantiation was deferred, and restoring it when doing this
deferred instantiation.  That way newly instantiated declarations
appropriately get a DECL_MODULE_PURVIEW_P appropriate for where the
instantiation was required, meaning that GMF entities won't be counted
as reachable unless referenced by an actually reachable entity.

Note that purviewness and attachment etc. is generally only determined
by the base template: this is purely for determining whether a
specialisation was declared in the module purview and hence whether it
should be streamed out.  See the comment on 'set_instantiating_module'.

	PR c++/114630
	PR c++/114795

gcc/cp/ChangeLog:

	* cp-tree.h (struct tinst_level): Add field for tracking
	module_kind.
	* pt.cc (push_tinst_level_loc): Cache module_kind in new_level.
	(reopen_tinst_level): Restore module_kind from level.
	(instantiate_pending_templates): Save and restore module_kind so
	it isn't affected by reopen_tinst_level.

gcc/testsuite/ChangeLog:

	* g++.dg/modules/gmf-3.C: New test.

Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
---
 gcc/cp/cp-tree.h                     |  3 +++
 gcc/cp/pt.cc                         |  4 ++++
 gcc/testsuite/g++.dg/modules/gmf-3.C | 13 +++++++++++++
 3 files changed, 20 insertions(+)
 create mode 100644 gcc/testsuite/g++.dg/modules/gmf-3.C

diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index 1938ada0268..0e619120ccc 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -6626,6 +6626,9 @@ struct GTY((chain_next ("%h.next"))) tinst_level {
   /* The location where the template is instantiated.  */
   location_t locus;
 
+  /* The module kind where the template is instantiated. */
+  unsigned module_kind;
+
   /* errorcount + sorrycount when we pushed this level.  */
   unsigned short errors;
 
diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
index 1c3eef60c06..401aa92bc3e 100644
--- a/gcc/cp/pt.cc
+++ b/gcc/cp/pt.cc
@@ -11277,6 +11277,7 @@ push_tinst_level_loc (tree tldcl, tree targs, location_t loc)
   new_level->tldcl = tldcl;
   new_level->targs = targs;
   new_level->locus = loc;
+  new_level->module_kind = module_kind;
   new_level->errors = errorcount + sorrycount;
   new_level->next = NULL;
   new_level->refcount = 0;
@@ -11345,6 +11346,7 @@ reopen_tinst_level (struct tinst_level *level)
   for (t = level; t; t = t->next)
     ++tinst_depth;
 
+  module_kind = level->module_kind;
   set_refcount_ptr (current_tinst_level, level);
   pop_tinst_level ();
   if (current_tinst_level)
@@ -27442,6 +27444,7 @@ instantiate_pending_templates (int retries)
 {
   int reconsider;
   location_t saved_loc = input_location;
+  unsigned saved_module_kind = module_kind;
 
   /* Instantiating templates may trigger vtable generation.  This in turn
      may require further template instantiations.  We place a limit here
@@ -27532,6 +27535,7 @@ instantiate_pending_templates (int retries)
   while (reconsider);
 
   input_location = saved_loc;
+  module_kind = saved_module_kind;
 }
 
 /* Substitute ARGVEC into T, which is a list of initializers for
diff --git a/gcc/testsuite/g++.dg/modules/gmf-3.C b/gcc/testsuite/g++.dg/modules/gmf-3.C
new file mode 100644
index 00000000000..e52ae904ea9
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/gmf-3.C
@@ -0,0 +1,13 @@
+// PR c++/114630
+// { dg-additional-options "-fmodules-ts -Wno-global-module -fdump-lang-module" }
+// { dg-module-cmi M }
+
+module;
+template <typename> struct allocator {
+  allocator() {}
+};
+template class allocator<wchar_t>;
+export module M;
+
+// The whole GMF should be discarded here
+// { dg-final { scan-lang-dump "Wrote 0 clusters" module } }
-- 
2.43.2

Re: [PATCH 2/4] c++/modules: Track module purview for deferred instantiations [PR114630]

[Patrick Palka]

On Wed, 1 May 2024, Nathaniel Shead wrote:

> Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
> 
> -- >8 --
> 
> When calling instantiate_pending_templates at end of parsing, any new
> functions that are instantiated from this point have their module
> purview set based on the current value of module_kind.
> 
> This is unideal, however, as the modules code will then treat these
> instantiations as reachable and cause large swathes of the GMF to be
> emitted into the module CMI, despite no code in the actual module
> purview referencing it.
> 
> This patch fixes this by also remembering the value of module_kind when
> the instantiation was deferred, and restoring it when doing this
> deferred instantiation.  That way newly instantiated declarations
> appropriately get a DECL_MODULE_PURVIEW_P appropriate for where the
> instantiation was required, meaning that GMF entities won't be counted
> as reachable unless referenced by an actually reachable entity.
> 
> Note that purviewness and attachment etc. is generally only determined
> by the base template: this is purely for determining whether a
> specialisation was declared in the module purview and hence whether it
> should be streamed out.  See the comment on 'set_instantiating_module'.
> 
> 	PR c++/114630
> 	PR c++/114795
> 
> gcc/cp/ChangeLog:
> 
> 	* cp-tree.h (struct tinst_level): Add field for tracking
> 	module_kind.
> 	* pt.cc (push_tinst_level_loc): Cache module_kind in new_level.
> 	(reopen_tinst_level): Restore module_kind from level.
> 	(instantiate_pending_templates): Save and restore module_kind so
> 	it isn't affected by reopen_tinst_level.

LGTM.  Another approach is to instantiate all so-far deferred
instantiations and vtables once we reach the start of the module
purview, but your approach is much cleaner.

Note that deferred instantiation can induce more deferred instantiation,
but your approach will do the right thing here (module_kind will be
inherited from the point of instantiation).

What if an instantiation is needed from both the GMF and the module
purview?  Then IIUC it'll be instantiated as if from the GMF, which
seems right to me.

> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/modules/gmf-3.C: New test.
> 
> Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
> ---
>  gcc/cp/cp-tree.h                     |  3 +++
>  gcc/cp/pt.cc                         |  4 ++++
>  gcc/testsuite/g++.dg/modules/gmf-3.C | 13 +++++++++++++
>  3 files changed, 20 insertions(+)
>  create mode 100644 gcc/testsuite/g++.dg/modules/gmf-3.C
> 
> diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
> index 1938ada0268..0e619120ccc 100644
> --- a/gcc/cp/cp-tree.h
> +++ b/gcc/cp/cp-tree.h
> @@ -6626,6 +6626,9 @@ struct GTY((chain_next ("%h.next"))) tinst_level {
>    /* The location where the template is instantiated.  */
>    location_t locus;
>  
> +  /* The module kind where the template is instantiated. */
> +  unsigned module_kind;
> +
>    /* errorcount + sorrycount when we pushed this level.  */
>    unsigned short errors;
>  
> diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
> index 1c3eef60c06..401aa92bc3e 100644
> --- a/gcc/cp/pt.cc
> +++ b/gcc/cp/pt.cc
> @@ -11277,6 +11277,7 @@ push_tinst_level_loc (tree tldcl, tree targs, location_t loc)
>    new_level->tldcl = tldcl;
>    new_level->targs = targs;
>    new_level->locus = loc;
> +  new_level->module_kind = module_kind;
>    new_level->errors = errorcount + sorrycount;
>    new_level->next = NULL;
>    new_level->refcount = 0;
> @@ -11345,6 +11346,7 @@ reopen_tinst_level (struct tinst_level *level)
>    for (t = level; t; t = t->next)
>      ++tinst_depth;
>  
> +  module_kind = level->module_kind;
>    set_refcount_ptr (current_tinst_level, level);
>    pop_tinst_level ();
>    if (current_tinst_level)
> @@ -27442,6 +27444,7 @@ instantiate_pending_templates (int retries)
>  {
>    int reconsider;
>    location_t saved_loc = input_location;
> +  unsigned saved_module_kind = module_kind;
>  
>    /* Instantiating templates may trigger vtable generation.  This in turn
>       may require further template instantiations.  We place a limit here
> @@ -27532,6 +27535,7 @@ instantiate_pending_templates (int retries)
>    while (reconsider);
>  
>    input_location = saved_loc;
> +  module_kind = saved_module_kind;
>  }
>  
>  /* Substitute ARGVEC into T, which is a list of initializers for
> diff --git a/gcc/testsuite/g++.dg/modules/gmf-3.C b/gcc/testsuite/g++.dg/modules/gmf-3.C
> new file mode 100644
> index 00000000000..e52ae904ea9
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/gmf-3.C
> @@ -0,0 +1,13 @@
> +// PR c++/114630
> +// { dg-additional-options "-fmodules-ts -Wno-global-module -fdump-lang-module" }
> +// { dg-module-cmi M }
> +
> +module;
> +template <typename> struct allocator {
> +  allocator() {}
> +};
> +template class allocator<wchar_t>;
> +export module M;
> +
> +// The whole GMF should be discarded here
> +// { dg-final { scan-lang-dump "Wrote 0 clusters" module } }
> -- 
> 2.43.2
> 
> 

Re: [PATCH 2/4] c++/modules: Track module purview for deferred instantiations [PR114630]

[Nathaniel Shead]

On Wed, May 01, 2024 at 10:11:20AM -0400, Patrick Palka wrote:
> On Wed, 1 May 2024, Nathaniel Shead wrote:
> 
> > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
> > 
> > -- >8 --
> > 
> > When calling instantiate_pending_templates at end of parsing, any new
> > functions that are instantiated from this point have their module
> > purview set based on the current value of module_kind.
> > 
> > This is unideal, however, as the modules code will then treat these
> > instantiations as reachable and cause large swathes of the GMF to be
> > emitted into the module CMI, despite no code in the actual module
> > purview referencing it.
> > 
> > This patch fixes this by also remembering the value of module_kind when
> > the instantiation was deferred, and restoring it when doing this
> > deferred instantiation.  That way newly instantiated declarations
> > appropriately get a DECL_MODULE_PURVIEW_P appropriate for where the
> > instantiation was required, meaning that GMF entities won't be counted
> > as reachable unless referenced by an actually reachable entity.
> > 
> > Note that purviewness and attachment etc. is generally only determined
> > by the base template: this is purely for determining whether a
> > specialisation was declared in the module purview and hence whether it
> > should be streamed out.  See the comment on 'set_instantiating_module'.
> > 
> > 	PR c++/114630
> > 	PR c++/114795
> > 
> > gcc/cp/ChangeLog:
> > 
> > 	* cp-tree.h (struct tinst_level): Add field for tracking
> > 	module_kind.
> > 	* pt.cc (push_tinst_level_loc): Cache module_kind in new_level.
> > 	(reopen_tinst_level): Restore module_kind from level.
> > 	(instantiate_pending_templates): Save and restore module_kind so
> > 	it isn't affected by reopen_tinst_level.
> 
> LGTM.  Another approach is to instantiate all so-far deferred
> instantiations and vtables once we reach the start of the module
> purview, but your approach is much cleaner.
> 
> Note that deferred instantiation can induce more deferred instantiation,
> but your approach will do the right thing here (module_kind will be
> inherited from the point of instantiation).
> 
> What if an instantiation is needed from both the GMF and the module
> purview?  Then IIUC it'll be instantiated as if from the GMF, which
> seems right to me.
> 

Hm..., so I believe it'll be marked according to whichever instantiates
it "first", so if e.g. deferred from GMF and then instantiated
non-deferred from purview it'll be marked purview (and the deferred
instantiation will be skipped as it's already instantiated).  This could
mean it gets unnecessarily emitted if it only got instantiated because
it got used in e.g. a non-inline function body, I suppose; but that's
true in general actually, at the moment.

Maybe then a better approach would be to instead always use the
DECL_MODULE_PURVIEW_P of the instantiating template instead of the
global 'module_purview_p' function in 'set_instantiating_module'?
I think that should still make sure that instantiations are emitted
when they need to be (because they'll be reachable from a declaration
in the purview): I might try to experiment with something like this.

> > 
> > gcc/testsuite/ChangeLog:
> > 
> > 	* g++.dg/modules/gmf-3.C: New test.
> > 
> > Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
> > ---
> >  gcc/cp/cp-tree.h                     |  3 +++
> >  gcc/cp/pt.cc                         |  4 ++++
> >  gcc/testsuite/g++.dg/modules/gmf-3.C | 13 +++++++++++++
> >  3 files changed, 20 insertions(+)
> >  create mode 100644 gcc/testsuite/g++.dg/modules/gmf-3.C
> > 
> > diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
> > index 1938ada0268..0e619120ccc 100644
> > --- a/gcc/cp/cp-tree.h
> > +++ b/gcc/cp/cp-tree.h
> > @@ -6626,6 +6626,9 @@ struct GTY((chain_next ("%h.next"))) tinst_level {
> >    /* The location where the template is instantiated.  */
> >    location_t locus;
> >  
> > +  /* The module kind where the template is instantiated. */
> > +  unsigned module_kind;
> > +
> >    /* errorcount + sorrycount when we pushed this level.  */
> >    unsigned short errors;
> >  
> > diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
> > index 1c3eef60c06..401aa92bc3e 100644
> > --- a/gcc/cp/pt.cc
> > +++ b/gcc/cp/pt.cc
> > @@ -11277,6 +11277,7 @@ push_tinst_level_loc (tree tldcl, tree targs, location_t loc)
> >    new_level->tldcl = tldcl;
> >    new_level->targs = targs;
> >    new_level->locus = loc;
> > +  new_level->module_kind = module_kind;
> >    new_level->errors = errorcount + sorrycount;
> >    new_level->next = NULL;
> >    new_level->refcount = 0;
> > @@ -11345,6 +11346,7 @@ reopen_tinst_level (struct tinst_level *level)
> >    for (t = level; t; t = t->next)
> >      ++tinst_depth;
> >  
> > +  module_kind = level->module_kind;
> >    set_refcount_ptr (current_tinst_level, level);
> >    pop_tinst_level ();
> >    if (current_tinst_level)
> > @@ -27442,6 +27444,7 @@ instantiate_pending_templates (int retries)
> >  {
> >    int reconsider;
> >    location_t saved_loc = input_location;
> > +  unsigned saved_module_kind = module_kind;
> >  
> >    /* Instantiating templates may trigger vtable generation.  This in turn
> >       may require further template instantiations.  We place a limit here
> > @@ -27532,6 +27535,7 @@ instantiate_pending_templates (int retries)
> >    while (reconsider);
> >  
> >    input_location = saved_loc;
> > +  module_kind = saved_module_kind;
> >  }
> >  
> >  /* Substitute ARGVEC into T, which is a list of initializers for
> > diff --git a/gcc/testsuite/g++.dg/modules/gmf-3.C b/gcc/testsuite/g++.dg/modules/gmf-3.C
> > new file mode 100644
> > index 00000000000..e52ae904ea9
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/modules/gmf-3.C
> > @@ -0,0 +1,13 @@
> > +// PR c++/114630
> > +// { dg-additional-options "-fmodules-ts -Wno-global-module -fdump-lang-module" }
> > +// { dg-module-cmi M }
> > +
> > +module;
> > +template <typename> struct allocator {
> > +  allocator() {}
> > +};
> > +template class allocator<wchar_t>;
> > +export module M;
> > +
> > +// The whole GMF should be discarded here
> > +// { dg-final { scan-lang-dump "Wrote 0 clusters" module } }
> > -- 
> > 2.43.2
> > 
> > 
> 

Re: [PATCH 2/4] c++/modules: Track module purview for deferred instantiations [PR114630]

[Patrick Palka]

On Thu, 2 May 2024, Nathaniel Shead wrote:

> On Wed, May 01, 2024 at 10:11:20AM -0400, Patrick Palka wrote:
> > On Wed, 1 May 2024, Nathaniel Shead wrote:
> > 
> > > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
> > > 
> > > -- >8 --
> > > 
> > > When calling instantiate_pending_templates at end of parsing, any new
> > > functions that are instantiated from this point have their module
> > > purview set based on the current value of module_kind.
> > > 
> > > This is unideal, however, as the modules code will then treat these
> > > instantiations as reachable and cause large swathes of the GMF to be
> > > emitted into the module CMI, despite no code in the actual module
> > > purview referencing it.
> > > 
> > > This patch fixes this by also remembering the value of module_kind when
> > > the instantiation was deferred, and restoring it when doing this
> > > deferred instantiation.  That way newly instantiated declarations
> > > appropriately get a DECL_MODULE_PURVIEW_P appropriate for where the
> > > instantiation was required, meaning that GMF entities won't be counted
> > > as reachable unless referenced by an actually reachable entity.
> > > 
> > > Note that purviewness and attachment etc. is generally only determined
> > > by the base template: this is purely for determining whether a
> > > specialisation was declared in the module purview and hence whether it
> > > should be streamed out.  See the comment on 'set_instantiating_module'.
> > > 
> > > 	PR c++/114630
> > > 	PR c++/114795
> > > 
> > > gcc/cp/ChangeLog:
> > > 
> > > 	* cp-tree.h (struct tinst_level): Add field for tracking
> > > 	module_kind.
> > > 	* pt.cc (push_tinst_level_loc): Cache module_kind in new_level.
> > > 	(reopen_tinst_level): Restore module_kind from level.
> > > 	(instantiate_pending_templates): Save and restore module_kind so
> > > 	it isn't affected by reopen_tinst_level.
> > 
> > LGTM.  Another approach is to instantiate all so-far deferred
> > instantiations and vtables once we reach the start of the module
> > purview, but your approach is much cleaner.
> > 
> > Note that deferred instantiation can induce more deferred instantiation,
> > but your approach will do the right thing here (module_kind will be
> > inherited from the point of instantiation).
> > 
> > What if an instantiation is needed from both the GMF and the module
> > purview?  Then IIUC it'll be instantiated as if from the GMF, which
> > seems right to me.
> > 
> 
> Hm..., so I believe it'll be marked according to whichever instantiates
> it "first", so if e.g. deferred from GMF and then instantiated
> non-deferred from purview it'll be marked purview (and the deferred
> instantiation will be skipped as it's already instantiated).  This could
> mean it gets unnecessarily emitted if it only got instantiated because
> it got used in e.g. a non-inline function body, I suppose; but that's
> true in general actually, at the moment.

FWIW I think the most relevant situation where we need to instantiate
immediately and can't defer until EOF is during constexpr evaluation of
a variable initializer.  (We also immediately instantiate variable
template specializations, and function template specializations with
deduced return type, but in these cases we'd always instantiate from the
first use, i.e. from the GMF in this scenario.)

So I guess a testcase for this situation could be something like:

    module;
    template<class T> constexpr int f(T t) { return t; }
    template int f(int);

    export module foo;
    constexpr int w = f(0);
    // f<int> ideally shouldn't be emitted in the CMI?

> 
> Maybe then a better approach would be to instead always use the
> DECL_MODULE_PURVIEW_P of the instantiating template instead of the
> global 'module_purview_p' function in 'set_instantiating_module'?
> I think that should still make sure that instantiations are emitted
> when they need to be (because they'll be reachable from a declaration
> in the purview): I might try to experiment with something like this.

The approach of instantiating all so-far deferred instantiations/vtables
at the start of the module purview should handle this properly too.

> 
> > > 
> > > gcc/testsuite/ChangeLog:
> > > 
> > > 	* g++.dg/modules/gmf-3.C: New test.
> > > 
> > > Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
> > > ---
> > >  gcc/cp/cp-tree.h                     |  3 +++
> > >  gcc/cp/pt.cc                         |  4 ++++
> > >  gcc/testsuite/g++.dg/modules/gmf-3.C | 13 +++++++++++++
> > >  3 files changed, 20 insertions(+)
> > >  create mode 100644 gcc/testsuite/g++.dg/modules/gmf-3.C
> > > 
> > > diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
> > > index 1938ada0268..0e619120ccc 100644
> > > --- a/gcc/cp/cp-tree.h
> > > +++ b/gcc/cp/cp-tree.h
> > > @@ -6626,6 +6626,9 @@ struct GTY((chain_next ("%h.next"))) tinst_level {
> > >    /* The location where the template is instantiated.  */
> > >    location_t locus;
> > >  
> > > +  /* The module kind where the template is instantiated. */
> > > +  unsigned module_kind;
> > > +
> > >    /* errorcount + sorrycount when we pushed this level.  */
> > >    unsigned short errors;
> > >  
> > > diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
> > > index 1c3eef60c06..401aa92bc3e 100644
> > > --- a/gcc/cp/pt.cc
> > > +++ b/gcc/cp/pt.cc
> > > @@ -11277,6 +11277,7 @@ push_tinst_level_loc (tree tldcl, tree targs, location_t loc)
> > >    new_level->tldcl = tldcl;
> > >    new_level->targs = targs;
> > >    new_level->locus = loc;
> > > +  new_level->module_kind = module_kind;
> > >    new_level->errors = errorcount + sorrycount;
> > >    new_level->next = NULL;
> > >    new_level->refcount = 0;
> > > @@ -11345,6 +11346,7 @@ reopen_tinst_level (struct tinst_level *level)
> > >    for (t = level; t; t = t->next)
> > >      ++tinst_depth;
> > >  
> > > +  module_kind = level->module_kind;
> > >    set_refcount_ptr (current_tinst_level, level);
> > >    pop_tinst_level ();
> > >    if (current_tinst_level)
> > > @@ -27442,6 +27444,7 @@ instantiate_pending_templates (int retries)
> > >  {
> > >    int reconsider;
> > >    location_t saved_loc = input_location;
> > > +  unsigned saved_module_kind = module_kind;
> > >  
> > >    /* Instantiating templates may trigger vtable generation.  This in turn
> > >       may require further template instantiations.  We place a limit here
> > > @@ -27532,6 +27535,7 @@ instantiate_pending_templates (int retries)
> > >    while (reconsider);
> > >  
> > >    input_location = saved_loc;
> > > +  module_kind = saved_module_kind;
> > >  }
> > >  
> > >  /* Substitute ARGVEC into T, which is a list of initializers for
> > > diff --git a/gcc/testsuite/g++.dg/modules/gmf-3.C b/gcc/testsuite/g++.dg/modules/gmf-3.C
> > > new file mode 100644
> > > index 00000000000..e52ae904ea9
> > > --- /dev/null
> > > +++ b/gcc/testsuite/g++.dg/modules/gmf-3.C
> > > @@ -0,0 +1,13 @@
> > > +// PR c++/114630
> > > +// { dg-additional-options "-fmodules-ts -Wno-global-module -fdump-lang-module" }
> > > +// { dg-module-cmi M }
> > > +
> > > +module;
> > > +template <typename> struct allocator {
> > > +  allocator() {}
> > > +};
> > > +template class allocator<wchar_t>;
> > > +export module M;
> > > +
> > > +// The whole GMF should be discarded here
> > > +// { dg-final { scan-lang-dump "Wrote 0 clusters" module } }
> > > -- 
> > > 2.43.2
> > > 
> > > 
> > 
> 
> 

Re: [PATCH 2/4] c++/modules: Track module purview for deferred instantiations [PR114630]

[Jason Merrill]

On 5/1/24 08:54, Patrick Palka wrote:
> On Thu, 2 May 2024, Nathaniel Shead wrote:
> 
>> On Wed, May 01, 2024 at 10:11:20AM -0400, Patrick Palka wrote:
>>> On Wed, 1 May 2024, Nathaniel Shead wrote:
>>>
>>>> Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
>>>>
>>>> -- >8 --
>>>>
>>>> When calling instantiate_pending_templates at end of parsing, any new
>>>> functions that are instantiated from this point have their module
>>>> purview set based on the current value of module_kind.
>>>>
>>>> This is unideal, however, as the modules code will then treat these
>>>> instantiations as reachable and cause large swathes of the GMF to be
>>>> emitted into the module CMI, despite no code in the actual module
>>>> purview referencing it.
>>>>
>>>> This patch fixes this by also remembering the value of module_kind when
>>>> the instantiation was deferred, and restoring it when doing this
>>>> deferred instantiation.  That way newly instantiated declarations
>>>> appropriately get a DECL_MODULE_PURVIEW_P appropriate for where the
>>>> instantiation was required, meaning that GMF entities won't be counted
>>>> as reachable unless referenced by an actually reachable entity.
>>>>
>>>> Note that purviewness and attachment etc. is generally only determined
>>>> by the base template: this is purely for determining whether a
>>>> specialisation was declared in the module purview and hence whether it
>>>> should be streamed out.  See the comment on 'set_instantiating_module'.
>>>>
>>>> 	PR c++/114630
>>>> 	PR c++/114795
>>>>
>>>> gcc/cp/ChangeLog:
>>>>
>>>> 	* cp-tree.h (struct tinst_level): Add field for tracking
>>>> 	module_kind.
>>>> 	* pt.cc (push_tinst_level_loc): Cache module_kind in new_level.
>>>> 	(reopen_tinst_level): Restore module_kind from level.
>>>> 	(instantiate_pending_templates): Save and restore module_kind so
>>>> 	it isn't affected by reopen_tinst_level.
>>>
>>> LGTM.  Another approach is to instantiate all so-far deferred
>>> instantiations and vtables once we reach the start of the module
>>> purview, but your approach is much cleaner.
>>>
>>> Note that deferred instantiation can induce more deferred instantiation,
>>> but your approach will do the right thing here (module_kind will be
>>> inherited from the point of instantiation).
>>>
>>> What if an instantiation is needed from both the GMF and the module
>>> purview?  Then IIUC it'll be instantiated as if from the GMF, which
>>> seems right to me.
>>>
>>
>> Hm..., so I believe it'll be marked according to whichever instantiates
>> it "first", so if e.g. deferred from GMF and then instantiated
>> non-deferred from purview it'll be marked purview (and the deferred
>> instantiation will be skipped as it's already instantiated).  This could
>> mean it gets unnecessarily emitted if it only got instantiated because
>> it got used in e.g. a non-inline function body, I suppose; but that's
>> true in general actually, at the moment.
> 
> FWIW I think the most relevant situation where we need to instantiate
> immediately and can't defer until EOF is during constexpr evaluation of
> a variable initializer.  (We also immediately instantiate variable
> template specializations, and function template specializations with
> deduced return type, but in these cases we'd always instantiate from the
> first use, i.e. from the GMF in this scenario.)
> 
> So I guess a testcase for this situation could be something like:
> 
>      module;
>      template<class T> constexpr int f(T t) { return t; }
>      template int f(int);
> 
>      export module foo;
>      constexpr int w = f(0);
>      // f<int> ideally shouldn't be emitted in the CMI?

Does the explicit instantiation change that at all?  Either way it seams 
like f and f<int> aren't decl-reachable from w if we determine that 
after constant evaluation.

Jason

Re: [PATCH 2/4] c++/modules: Track module purview for deferred instantiations [PR114630]

[Patrick Palka]

On Wed, 1 May 2024, Jason Merrill wrote:

> On 5/1/24 08:54, Patrick Palka wrote:
> > On Thu, 2 May 2024, Nathaniel Shead wrote:
> > 
> > > On Wed, May 01, 2024 at 10:11:20AM -0400, Patrick Palka wrote:
> > > > On Wed, 1 May 2024, Nathaniel Shead wrote:
> > > > 
> > > > > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
> > > > > 
> > > > > -- >8 --
> > > > > 
> > > > > When calling instantiate_pending_templates at end of parsing, any new
> > > > > functions that are instantiated from this point have their module
> > > > > purview set based on the current value of module_kind.
> > > > > 
> > > > > This is unideal, however, as the modules code will then treat these
> > > > > instantiations as reachable and cause large swathes of the GMF to be
> > > > > emitted into the module CMI, despite no code in the actual module
> > > > > purview referencing it.
> > > > > 
> > > > > This patch fixes this by also remembering the value of module_kind
> > > > > when
> > > > > the instantiation was deferred, and restoring it when doing this
> > > > > deferred instantiation.  That way newly instantiated declarations
> > > > > appropriately get a DECL_MODULE_PURVIEW_P appropriate for where the
> > > > > instantiation was required, meaning that GMF entities won't be counted
> > > > > as reachable unless referenced by an actually reachable entity.
> > > > > 
> > > > > Note that purviewness and attachment etc. is generally only determined
> > > > > by the base template: this is purely for determining whether a
> > > > > specialisation was declared in the module purview and hence whether it
> > > > > should be streamed out.  See the comment on
> > > > > 'set_instantiating_module'.
> > > > > 
> > > > > 	PR c++/114630
> > > > > 	PR c++/114795
> > > > > 
> > > > > gcc/cp/ChangeLog:
> > > > > 
> > > > > 	* cp-tree.h (struct tinst_level): Add field for tracking
> > > > > 	module_kind.
> > > > > 	* pt.cc (push_tinst_level_loc): Cache module_kind in
> > > > > new_level.
> > > > > 	(reopen_tinst_level): Restore module_kind from level.
> > > > > 	(instantiate_pending_templates): Save and restore module_kind
> > > > > so
> > > > > 	it isn't affected by reopen_tinst_level.
> > > > 
> > > > LGTM.  Another approach is to instantiate all so-far deferred
> > > > instantiations and vtables once we reach the start of the module
> > > > purview, but your approach is much cleaner.
> > > > 
> > > > Note that deferred instantiation can induce more deferred instantiation,
> > > > but your approach will do the right thing here (module_kind will be
> > > > inherited from the point of instantiation).
> > > > 
> > > > What if an instantiation is needed from both the GMF and the module
> > > > purview?  Then IIUC it'll be instantiated as if from the GMF, which
> > > > seems right to me.
> > > > 
> > > 
> > > Hm..., so I believe it'll be marked according to whichever instantiates
> > > it "first", so if e.g. deferred from GMF and then instantiated
> > > non-deferred from purview it'll be marked purview (and the deferred
> > > instantiation will be skipped as it's already instantiated).  This could
> > > mean it gets unnecessarily emitted if it only got instantiated because
> > > it got used in e.g. a non-inline function body, I suppose; but that's
> > > true in general actually, at the moment.
> > 
> > FWIW I think the most relevant situation where we need to instantiate
> > immediately and can't defer until EOF is during constexpr evaluation of
> > a variable initializer.  (We also immediately instantiate variable
> > template specializations, and function template specializations with
> > deduced return type, but in these cases we'd always instantiate from the
> > first use, i.e. from the GMF in this scenario.)
> > 
> > So I guess a testcase for this situation could be something like:
> > 
> >      module;
> >      template<class T> constexpr int f(T t) { return t; }
> >      template int f(int);
> > 
> >      export module foo;
> >      constexpr int w = f(0);
> >      // f<int> ideally shouldn't be emitted in the CMI?
> 
> Does the explicit instantiation change that at all?  Either way it seams like
> f and f<int> aren't decl-reachable from w if we determine that after constant
> evaluation.

Currently due to the bug in question we handle this as if the explicit
instantiation was performed from the module purview:

  module;
  template<class T> constexpr int f(T t) { return t; }

  export module foo;
  template int f(int);
  constexpr int w = f(0);

I think we do want to emit f<int> in that case?

Re: [PATCH 2/4] c++/modules: Track module purview for deferred instantiations [PR114630]

[Jason Merrill]

On 5/1/24 12:11, Patrick Palka wrote:
> On Wed, 1 May 2024, Jason Merrill wrote:
> 
>> On 5/1/24 08:54, Patrick Palka wrote:
>>> On Thu, 2 May 2024, Nathaniel Shead wrote:
>>>
>>>> On Wed, May 01, 2024 at 10:11:20AM -0400, Patrick Palka wrote:
>>>>> On Wed, 1 May 2024, Nathaniel Shead wrote:
>>>>>
>>>>>> Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
>>>>>>
>>>>>> -- >8 --
>>>>>>
>>>>>> When calling instantiate_pending_templates at end of parsing, any new
>>>>>> functions that are instantiated from this point have their module
>>>>>> purview set based on the current value of module_kind.
>>>>>>
>>>>>> This is unideal, however, as the modules code will then treat these
>>>>>> instantiations as reachable and cause large swathes of the GMF to be
>>>>>> emitted into the module CMI, despite no code in the actual module
>>>>>> purview referencing it.
>>>>>>
>>>>>> This patch fixes this by also remembering the value of module_kind
>>>>>> when
>>>>>> the instantiation was deferred, and restoring it when doing this
>>>>>> deferred instantiation.  That way newly instantiated declarations
>>>>>> appropriately get a DECL_MODULE_PURVIEW_P appropriate for where the
>>>>>> instantiation was required, meaning that GMF entities won't be counted
>>>>>> as reachable unless referenced by an actually reachable entity.
>>>>>>
>>>>>> Note that purviewness and attachment etc. is generally only determined
>>>>>> by the base template: this is purely for determining whether a
>>>>>> specialisation was declared in the module purview and hence whether it
>>>>>> should be streamed out.  See the comment on
>>>>>> 'set_instantiating_module'.
>>>>>>
>>>>>> 	PR c++/114630
>>>>>> 	PR c++/114795
>>>>>>
>>>>>> gcc/cp/ChangeLog:
>>>>>>
>>>>>> 	* cp-tree.h (struct tinst_level): Add field for tracking
>>>>>> 	module_kind.
>>>>>> 	* pt.cc (push_tinst_level_loc): Cache module_kind in
>>>>>> new_level.
>>>>>> 	(reopen_tinst_level): Restore module_kind from level.
>>>>>> 	(instantiate_pending_templates): Save and restore module_kind
>>>>>> so
>>>>>> 	it isn't affected by reopen_tinst_level.
>>>>>
>>>>> LGTM.  Another approach is to instantiate all so-far deferred
>>>>> instantiations and vtables once we reach the start of the module
>>>>> purview, but your approach is much cleaner.
>>>>>
>>>>> Note that deferred instantiation can induce more deferred instantiation,
>>>>> but your approach will do the right thing here (module_kind will be
>>>>> inherited from the point of instantiation).
>>>>>
>>>>> What if an instantiation is needed from both the GMF and the module
>>>>> purview?  Then IIUC it'll be instantiated as if from the GMF, which
>>>>> seems right to me.
>>>>>
>>>>
>>>> Hm..., so I believe it'll be marked according to whichever instantiates
>>>> it "first", so if e.g. deferred from GMF and then instantiated
>>>> non-deferred from purview it'll be marked purview (and the deferred
>>>> instantiation will be skipped as it's already instantiated).  This could
>>>> mean it gets unnecessarily emitted if it only got instantiated because
>>>> it got used in e.g. a non-inline function body, I suppose; but that's
>>>> true in general actually, at the moment.
>>>
>>> FWIW I think the most relevant situation where we need to instantiate
>>> immediately and can't defer until EOF is during constexpr evaluation of
>>> a variable initializer.  (We also immediately instantiate variable
>>> template specializations, and function template specializations with
>>> deduced return type, but in these cases we'd always instantiate from the
>>> first use, i.e. from the GMF in this scenario.)
>>>
>>> So I guess a testcase for this situation could be something like:
>>>
>>>       module;
>>>       template<class T> constexpr int f(T t) { return t; }
>>>       template int f(int);
>>>
>>>       export module foo;
>>>       constexpr int w = f(0);
>>>       // f<int> ideally shouldn't be emitted in the CMI?
>>
>> Does the explicit instantiation change that at all?  Either way it seams like
>> f and f<int> aren't decl-reachable from w if we determine that after constant
>> evaluation.
> 
> Currently due to the bug in question we handle this as if the explicit
> instantiation was performed from the module purview:
> 
>    module;
>    template<class T> constexpr int f(T t) { return t; }
> 
>    export module foo;
>    template int f(int);
>    constexpr int w = f(0);
> 
> I think we do want to emit f<int> in that case?

Agreed, because f is decl-reachable from the explicit instantiation in 
the module purview.

I was suggesting that there's no effective difference between explicit 
instantiation in the GMF and implicit instantiation.

Jason

Re: [PATCH 2/4] c++/modules: Track module purview for deferred instantiations [PR114630]

[Jason Merrill]

On 5/1/24 07:11, Patrick Palka wrote:
> On Wed, 1 May 2024, Nathaniel Shead wrote:
> 
>> Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
>>
>> -- >8 --
>>
>> When calling instantiate_pending_templates at end of parsing, any new
>> functions that are instantiated from this point have their module
>> purview set based on the current value of module_kind.
>>
>> This is unideal, however, as the modules code will then treat these
>> instantiations as reachable and cause large swathes of the GMF to be
>> emitted into the module CMI, despite no code in the actual module
>> purview referencing it.
>>
>> This patch fixes this by also remembering the value of module_kind when
>> the instantiation was deferred, and restoring it when doing this
>> deferred instantiation.  That way newly instantiated declarations
>> appropriately get a DECL_MODULE_PURVIEW_P appropriate for where the
>> instantiation was required, meaning that GMF entities won't be counted
>> as reachable unless referenced by an actually reachable entity.
>>
>> Note that purviewness and attachment etc. is generally only determined
>> by the base template: this is purely for determining whether a
>> specialisation was declared in the module purview and hence whether it
>> should be streamed out.  See the comment on 'set_instantiating_module'.
>>
>> 	PR c++/114630
>> 	PR c++/114795
>>
>> gcc/cp/ChangeLog:
>>
>> 	* cp-tree.h (struct tinst_level): Add field for tracking
>> 	module_kind.
>> 	* pt.cc (push_tinst_level_loc): Cache module_kind in new_level.
>> 	(reopen_tinst_level): Restore module_kind from level.
>> 	(instantiate_pending_templates): Save and restore module_kind so
>> 	it isn't affected by reopen_tinst_level.
> 
> LGTM.  Another approach is to instantiate all so-far deferred
> instantiations and vtables once we reach the start of the module
> purview, but your approach is much cleaner.

Hmm, I actually think I like the approach of instantiating at that point 
better, so that instantiations in the GMF can't depend on things in 
module purview.

And probably do the same again when switching to the private module 
fragment, when that's implemented.

Jason

[PATCH 3/4] c++/modules: Also track module purview from deferred vtable instantiation [PR114630]

[Nathaniel Shead]

Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?

-- >8 --

Similarly to in the previous commit, templated virtual functions are
sometimes not instantiated until after parsing has completed.  This
means that any new declarations they instantiate are incorrectly marked
as being in the module purview, causing them to not be discarded from
the GMF.

	PR c++/114630
	PR c++/114795

gcc/cp/ChangeLog:

	* decl2.cc (mark_vtable_entries): Set module purview of deferred
	instantiations from the function decl.
	(c_parse_final_cleanups): Save and restore module_kind.

gcc/testsuite/ChangeLog:

	* g++.dg/modules/gmf-4.C: New test.

Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
---
 gcc/cp/decl2.cc                      | 11 +++++++++--
 gcc/testsuite/g++.dg/modules/gmf-4.C | 27 +++++++++++++++++++++++++++
 2 files changed, 36 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/modules/gmf-4.C

diff --git a/gcc/cp/decl2.cc b/gcc/cp/decl2.cc
index 806a2a4bc69..0115a6b1cc9 100644
--- a/gcc/cp/decl2.cc
+++ b/gcc/cp/decl2.cc
@@ -2210,6 +2210,11 @@ mark_vtable_entries (tree decl, vec<tree> &consteval_vtables)
          location, as we're called from c_parse_final_cleanups, which
          takes care of that.  */
       input_location = DECL_SOURCE_LOCATION (fn);
+      /* Similarly, any deferred instantiations should have the same
+	 module purview as this function.  */
+      module_kind &= ~MK_PURVIEW;
+      if (DECL_LANG_SPECIFIC (fn) && DECL_MODULE_PURVIEW_P (fn))
+	module_kind |= MK_PURVIEW;
       mark_used (fn);
     }
 }
@@ -5085,6 +5090,7 @@ c_parse_final_cleanups (void)
   tree decl;
 
   locus_at_end_of_parsing = input_location;
+  unsigned module_kind_at_end_of_parsing = module_kind;
   /* We're done parsing.  */
   at_eof = 1;
 
@@ -5177,9 +5183,10 @@ c_parse_final_cleanups (void)
 	    reconsider = true;
 	    keyed_classes->unordered_remove (i);
 	  }
-      /* The input_location may have been changed during marking of
-	 vtable entries.  */
+      /* The input_location and module_kind may have been changed
+	 during marking of vtable entries.  */
       input_location = locus_at_end_of_parsing;
+      module_kind = module_kind_at_end_of_parsing;
 
       /* Write out needed type info variables.  We have to be careful
 	 looping through unemitted decls, because emit_tinfo_decl may
diff --git a/gcc/testsuite/g++.dg/modules/gmf-4.C b/gcc/testsuite/g++.dg/modules/gmf-4.C
new file mode 100644
index 00000000000..c95bc782cea
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/gmf-4.C
@@ -0,0 +1,27 @@
+// PR c++/114630
+// { dg-additional-options "-fmodules-ts -Wno-global-module -fdump-lang-module" }
+// { dg-module-cmi M }
+
+// Deferred instantiation of GM virtual functions should not place
+// newly discovered declarations in the module purview
+
+module;
+
+template <typename T>
+void go() {
+  extern T fn_decl();
+}
+
+template <typename T>
+struct S {
+  virtual void f() {
+    go<char>();
+  }
+};
+
+S<int> s;
+
+export module M;
+
+// The whole GMF should be discarded here
+// { dg-final { scan-lang-dump "Wrote 0 clusters" module } }
-- 
2.43.2

Re: [PATCH 3/4] c++/modules: Also track module purview from deferred vtable instantiation [PR114630]

[Patrick Palka]

On Wed, 1 May 2024, Nathaniel Shead wrote:

> Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?

LGTM

> 
> -- >8 --
> 
> Similarly to in the previous commit, templated virtual functions are
> sometimes not instantiated until after parsing has completed.  This
> means that any new declarations they instantiate are incorrectly marked
> as being in the module purview, causing them to not be discarded from
> the GMF.
> 
> 	PR c++/114630
> 	PR c++/114795
> 
> gcc/cp/ChangeLog:
> 
> 	* decl2.cc (mark_vtable_entries): Set module purview of deferred
> 	instantiations from the function decl.
> 	(c_parse_final_cleanups): Save and restore module_kind.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/modules/gmf-4.C: New test.
> 
> Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
> ---
>  gcc/cp/decl2.cc                      | 11 +++++++++--
>  gcc/testsuite/g++.dg/modules/gmf-4.C | 27 +++++++++++++++++++++++++++
>  2 files changed, 36 insertions(+), 2 deletions(-)
>  create mode 100644 gcc/testsuite/g++.dg/modules/gmf-4.C
> 
> diff --git a/gcc/cp/decl2.cc b/gcc/cp/decl2.cc
> index 806a2a4bc69..0115a6b1cc9 100644
> --- a/gcc/cp/decl2.cc
> +++ b/gcc/cp/decl2.cc
> @@ -2210,6 +2210,11 @@ mark_vtable_entries (tree decl, vec<tree> &consteval_vtables)
>           location, as we're called from c_parse_final_cleanups, which
>           takes care of that.  */
>        input_location = DECL_SOURCE_LOCATION (fn);
> +      /* Similarly, any deferred instantiations should have the same
> +	 module purview as this function.  */
> +      module_kind &= ~MK_PURVIEW;
> +      if (DECL_LANG_SPECIFIC (fn) && DECL_MODULE_PURVIEW_P (fn))
> +	module_kind |= MK_PURVIEW;
>        mark_used (fn);
>      }
>  }
> @@ -5085,6 +5090,7 @@ c_parse_final_cleanups (void)
>    tree decl;
>  
>    locus_at_end_of_parsing = input_location;
> +  unsigned module_kind_at_end_of_parsing = module_kind;
>    /* We're done parsing.  */
>    at_eof = 1;
>  
> @@ -5177,9 +5183,10 @@ c_parse_final_cleanups (void)
>  	    reconsider = true;
>  	    keyed_classes->unordered_remove (i);
>  	  }
> -      /* The input_location may have been changed during marking of
> -	 vtable entries.  */
> +      /* The input_location and module_kind may have been changed
> +	 during marking of vtable entries.  */
>        input_location = locus_at_end_of_parsing;
> +      module_kind = module_kind_at_end_of_parsing;
>  
>        /* Write out needed type info variables.  We have to be careful
>  	 looping through unemitted decls, because emit_tinfo_decl may
> diff --git a/gcc/testsuite/g++.dg/modules/gmf-4.C b/gcc/testsuite/g++.dg/modules/gmf-4.C
> new file mode 100644
> index 00000000000..c95bc782cea
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/gmf-4.C
> @@ -0,0 +1,27 @@
> +// PR c++/114630
> +// { dg-additional-options "-fmodules-ts -Wno-global-module -fdump-lang-module" }
> +// { dg-module-cmi M }
> +
> +// Deferred instantiation of GM virtual functions should not place
> +// newly discovered declarations in the module purview
> +
> +module;
> +
> +template <typename T>
> +void go() {
> +  extern T fn_decl();
> +}
> +
> +template <typename T>
> +struct S {
> +  virtual void f() {
> +    go<char>();
> +  }
> +};
> +
> +S<int> s;
> +
> +export module M;
> +
> +// The whole GMF should be discarded here
> +// { dg-final { scan-lang-dump "Wrote 0 clusters" module } }
> -- 
> 2.43.2
> 
> 

[PATCH 4/4] c++: Add new xtreme-header testcase for GMF discarding

[Nathaniel Shead]

Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?

Another thought would be to replace xtreme-header.h with <bits/stdc++.h> so
that we don't need to keep it up-to-date in the future.  But this patch just
does the obviously correct thing.

-- >8 --

This new testcase helps verify that the issues discovered in PR114630
with GMF entries being inappropriately emitted into the module CMI don't
regress, at least for the kinds of code used in the standard library
headers.

This also updates the base xtreme-header.h file to include new header
files that are now available.

gcc/testsuite/ChangeLog:

	* g++.dg/modules/xtreme-header.h: Update.
	* g++.dg/modules/xtreme-header-8.C: New test.

Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
---
 .../g++.dg/modules/xtreme-header-8.C          |  9 +++++++
 gcc/testsuite/g++.dg/modules/xtreme-header.h  | 24 ++++++++++++-------
 2 files changed, 25 insertions(+), 8 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/modules/xtreme-header-8.C

diff --git a/gcc/testsuite/g++.dg/modules/xtreme-header-8.C b/gcc/testsuite/g++.dg/modules/xtreme-header-8.C
new file mode 100644
index 00000000000..b0d0ae87534
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/xtreme-header-8.C
@@ -0,0 +1,9 @@
+// { dg-additional-options "-fmodules-ts -fdump-lang-module" }
+// { dg-module-cmi empty }
+
+module;
+#include "xtreme-header.h"
+export module empty;
+
+// The whole GMF should be discarded here
+// { dg-final { scan-lang-dump "Wrote 0 clusters" module } }
diff --git a/gcc/testsuite/g++.dg/modules/xtreme-header.h b/gcc/testsuite/g++.dg/modules/xtreme-header.h
index 3147aaf00f4..8d95218755a 100644
--- a/gcc/testsuite/g++.dg/modules/xtreme-header.h
+++ b/gcc/testsuite/g++.dg/modules/xtreme-header.h
@@ -89,7 +89,6 @@
 
 // C++20
 #if __cplusplus > 201703
-#if 1
 #include <version>
 #include <barrier>
 #include <bit>
@@ -98,6 +97,7 @@
 #if __cpp_coroutines
 #include <coroutine>
 #endif
+#include <format>
 #include <latch>
 #include <numbers>
 #include <ranges>
@@ -106,24 +106,32 @@
 #include <span>
 #include <stop_token>
 #include <syncstream>
-#if 0
-// Unimplemented
-#include <format>
-#endif
-#endif
 #endif
 
 // C++23
 #if __cplusplus > 202002L
 #include <expected>
+#include <generator>
+#include <print>
 #include <spanstream>
 #include <stacktrace>
+#include <stdfloat>
 #if 0
 // Unimplemented
 #include <flat_map>
 #include <flat_set>
-#include <generator>
 #include <mdspan>
-#include <print>
+#endif
+#endif
+
+// C++26
+#if __cplusplus > 202302L
+#if 0
+// Unimplemented
+#include <debugging>
+#include <hazard_pointer>
+#include <linalg>
+#include <rcu>
+#include <text_encoding>
 #endif
 #endif
-- 
2.43.2

Re: [PATCH 4/4] c++: Add new xtreme-header testcase for GMF discarding

[Jason Merrill]

On 5/1/24 03:01, Nathaniel Shead wrote:
> Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?

OK.

> Another thought would be to replace xtreme-header.h with <bits/stdc++.h> so
> that we don't need to keep it up-to-date in the future.  But this patch just
> does the obviously correct thing.
> 
> -- >8 --
> 
> This new testcase helps verify that the issues discovered in PR114630
> with GMF entries being inappropriately emitted into the module CMI don't
> regress, at least for the kinds of code used in the standard library
> headers.
> 
> This also updates the base xtreme-header.h file to include new header
> files that are now available.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/modules/xtreme-header.h: Update.
> 	* g++.dg/modules/xtreme-header-8.C: New test.
> 
> Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
> ---
>   .../g++.dg/modules/xtreme-header-8.C          |  9 +++++++
>   gcc/testsuite/g++.dg/modules/xtreme-header.h  | 24 ++++++++++++-------
>   2 files changed, 25 insertions(+), 8 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/modules/xtreme-header-8.C
> 
> diff --git a/gcc/testsuite/g++.dg/modules/xtreme-header-8.C b/gcc/testsuite/g++.dg/modules/xtreme-header-8.C
> new file mode 100644
> index 00000000000..b0d0ae87534
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/xtreme-header-8.C
> @@ -0,0 +1,9 @@
> +// { dg-additional-options "-fmodules-ts -fdump-lang-module" }
> +// { dg-module-cmi empty }
> +
> +module;
> +#include "xtreme-header.h"
> +export module empty;
> +
> +// The whole GMF should be discarded here
> +// { dg-final { scan-lang-dump "Wrote 0 clusters" module } }
> diff --git a/gcc/testsuite/g++.dg/modules/xtreme-header.h b/gcc/testsuite/g++.dg/modules/xtreme-header.h
> index 3147aaf00f4..8d95218755a 100644
> --- a/gcc/testsuite/g++.dg/modules/xtreme-header.h
> +++ b/gcc/testsuite/g++.dg/modules/xtreme-header.h
> @@ -89,7 +89,6 @@
>   
>   // C++20
>   #if __cplusplus > 201703
> -#if 1
>   #include <version>
>   #include <barrier>
>   #include <bit>
> @@ -98,6 +97,7 @@
>   #if __cpp_coroutines
>   #include <coroutine>
>   #endif
> +#include <format>
>   #include <latch>
>   #include <numbers>
>   #include <ranges>
> @@ -106,24 +106,32 @@
>   #include <span>
>   #include <stop_token>
>   #include <syncstream>
> -#if 0
> -// Unimplemented
> -#include <format>
> -#endif
> -#endif
>   #endif
>   
>   // C++23
>   #if __cplusplus > 202002L
>   #include <expected>
> +#include <generator>
> +#include <print>
>   #include <spanstream>
>   #include <stacktrace>
> +#include <stdfloat>
>   #if 0
>   // Unimplemented
>   #include <flat_map>
>   #include <flat_set>
> -#include <generator>
>   #include <mdspan>
> -#include <print>
> +#endif
> +#endif
> +
> +// C++26
> +#if __cplusplus > 202302L
> +#if 0
> +// Unimplemented
> +#include <debugging>
> +#include <hazard_pointer>
> +#include <linalg>
> +#include <rcu>
> +#include <text_encoding>
>   #endif
>   #endif