diff mbox series

c++: Revert deferring emission of inline variables [PR114013]

Message ID 65d5d052.170a0220.f2168.38ba@mx.google.com
State New
Headers show
Series c++: Revert deferring emission of inline variables [PR114013] | expand

Commit Message

Nathaniel Shead Feb. 21, 2024, 10:28 a.m. UTC 
My earlier patch appears to have caused some regressions. I've taken a
quick look to see if there are obvious workarounds, but given the time
frame and the fact that I still don't really understand all the details
of how and when symbols get emitted, I felt it was safer to revert the
non-modules parts of this change instead.

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

-- >8 --

This is a (partial) reversion of r14-8987-gdd9d14f7d53 to return to
eagerly emitting inline variables to the middle-end when they are
declared. 'import_export_decl' will still continue to accept them, as
allowing this is a pure extension and doesn't seem to cause issues with
modules, but otherwise deferring the emission of inline variables
appears to cause issues on some targets and prevents some code using
inline variable templates from correctly linking.

There might be a more targetted way to fix this, but due to the
complexity of handling linkage I'd prefer to wait till GCC 15 to explore
our options.

	PR c++/113970
	PR c++/114013

gcc/cp/ChangeLog:

	* decl.cc (make_rtl_for_nonlocal_decl): Don't defer inline
	variables.
	* decl2.cc (import_export_decl): Only support inline variables
	imported from a module.

gcc/testsuite/ChangeLog:

	* g++.dg/cpp1z/inline-var10.C: New test.
---
 gcc/cp/decl.cc                            |  4 ---
 gcc/cp/decl2.cc                           |  6 +++--
 gcc/testsuite/g++.dg/cpp1z/inline-var10.C | 33 +++++++++++++++++++++++
 3 files changed, 37 insertions(+), 6 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/cpp1z/inline-var10.C

Comments

Patrick Palka Feb. 26, 2024, 4:59 p.m. UTC | #1 
On Wed, 21 Feb 2024, Nathaniel Shead wrote:

> My earlier patch appears to have caused some regressions. I've taken a
> quick look to see if there are obvious workarounds, but given the time
> frame and the fact that I still don't really understand all the details
> of how and when symbols get emitted, I felt it was safer to revert the
> non-modules parts of this change instead.
> 
> Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
> 
> -- >8 --
> 
> This is a (partial) reversion of r14-8987-gdd9d14f7d53 to return to
> eagerly emitting inline variables to the middle-end when they are
> declared. 'import_export_decl' will still continue to accept them, as
> allowing this is a pure extension and doesn't seem to cause issues with
> modules, but otherwise deferring the emission of inline variables
> appears to cause issues on some targets and prevents some code using
> inline variable templates from correctly linking.
> 
> There might be a more targetted way to fix this, but due to the
> complexity of handling linkage I'd prefer to wait till GCC 15 to explore
> our options.

Seems reasonable to me.

Aside from reverting, I suppose a couple of other options (which you've
probably already considered) are to restrict that defer_p condition
to non-templates by checking !DECL_USE_TEMPLATE (so in particular
explicit specializations aren't deferred), or to call
note_vague_linkage_variable for inline explicit specializations (e.g.
from make_rtl_for_nonlocal_decl upon deferring, or from
check_explicit_specialization).  These should help with PR114013, no
idea about PR113970 though..

> 
> 	PR c++/113970
> 	PR c++/114013
> 
> gcc/cp/ChangeLog:
> 
> 	* decl.cc (make_rtl_for_nonlocal_decl): Don't defer inline
> 	variables.
> 	* decl2.cc (import_export_decl): Only support inline variables
> 	imported from a module.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/cpp1z/inline-var10.C: New test.
> ---
>  gcc/cp/decl.cc                            |  4 ---
>  gcc/cp/decl2.cc                           |  6 +++--
>  gcc/testsuite/g++.dg/cpp1z/inline-var10.C | 33 +++++++++++++++++++++++
>  3 files changed, 37 insertions(+), 6 deletions(-)
>  create mode 100644 gcc/testsuite/g++.dg/cpp1z/inline-var10.C
> 
> diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc
> index e47f694e4e5..d19d09adde4 100644
> --- a/gcc/cp/decl.cc
> +++ b/gcc/cp/decl.cc
> @@ -7954,10 +7954,6 @@ make_rtl_for_nonlocal_decl (tree decl, tree init, const char* asmspec)
>        && DECL_IMPLICIT_INSTANTIATION (decl))
>      defer_p = 1;
>  
> -  /* Defer vague-linkage variables.  */
> -  if (DECL_INLINE_VAR_P (decl))
> -    defer_p = 1;
> -
>    /* If we're not deferring, go ahead and assemble the variable.  */
>    if (!defer_p)
>      rest_of_decl_compilation (decl, toplev, at_eof);
> diff --git a/gcc/cp/decl2.cc b/gcc/cp/decl2.cc
> index 1dddbaab38b..24a9332ccb1 100644
> --- a/gcc/cp/decl2.cc
> +++ b/gcc/cp/decl2.cc
> @@ -3362,7 +3362,7 @@ import_export_decl (tree decl)
>  
>       * inline functions
>  
> -     * inline variables
> +     * inline variables (from modules)
>  
>       * implicit instantiations of static data members of class
>         templates
> @@ -3385,7 +3385,9 @@ import_export_decl (tree decl)
>  		|| DECL_DECLARED_INLINE_P (decl));
>    else
>      gcc_assert (DECL_IMPLICIT_INSTANTIATION (decl)
> -		|| DECL_INLINE_VAR_P (decl)
> +		|| (DECL_INLINE_VAR_P (decl)
> +		    && DECL_LANG_SPECIFIC (decl)
> +		    && DECL_MODULE_IMPORT_P (decl))
>  		|| DECL_VTABLE_OR_VTT_P (decl)
>  		|| DECL_TINFO_P (decl));
>    /* Check that a definition of DECL is available in this translation
> diff --git a/gcc/testsuite/g++.dg/cpp1z/inline-var10.C b/gcc/testsuite/g++.dg/cpp1z/inline-var10.C
> new file mode 100644
> index 00000000000..8a198556778
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp1z/inline-var10.C
> @@ -0,0 +1,33 @@
> +// PR c++/114013
> +// { dg-do link { target c++17 } }
> +
> +struct S { int a, b; };
> +
> +template <int N>
> +constexpr struct S var[8] = {};
> +
> +template <>
> +constexpr inline struct S var<6>[8] = {
> +  { 1, 1 }, { 2, 0 }, { 3, 1 }, { 4, 0 },
> +  { 5, 1 }, { 6, 0 }, { 7, 1 }, { 8, 0 }
> +};
> +
> +[[gnu::noipa]] void
> +foo (S)
> +{
> +}
> +
> +template <int N>
> +void
> +bar (int x)
> +{
> +  foo (var<N>[x]);
> +}
> +
> +volatile int x;
> +
> +int
> +main ()
> +{
> +  bar <6> (x);
> +}
> -- 
> 2.43.0
> 
>
Jason Merrill Feb. 27, 2024, 1:46 a.m. UTC | #2 
On 2/21/24 05:28, Nathaniel Shead wrote:
> My earlier patch appears to have caused some regressions. I've taken a
> quick look to see if there are obvious workarounds, but given the time
> frame and the fact that I still don't really understand all the details
> of how and when symbols get emitted, I felt it was safer to revert the
> non-modules parts of this change instead.
> 
> Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?

The make_rtl_for_nonlocal_decl hunk is OK, I don't see the need to 
change import_export_decl.

> -- >8 --
> 
> This is a (partial) reversion of r14-8987-gdd9d14f7d53 to return to
> eagerly emitting inline variables to the middle-end when they are
> declared. 'import_export_decl' will still continue to accept them, as
> allowing this is a pure extension and doesn't seem to cause issues with
> modules, but otherwise deferring the emission of inline variables
> appears to cause issues on some targets and prevents some code using
> inline variable templates from correctly linking.
> 
> There might be a more targetted way to fix this, but due to the
> complexity of handling linkage I'd prefer to wait till GCC 15 to explore
> our options.
> 
> 	PR c++/113970
> 	PR c++/114013
> 
> gcc/cp/ChangeLog:
> 
> 	* decl.cc (make_rtl_for_nonlocal_decl): Don't defer inline
> 	variables.
> 	* decl2.cc (import_export_decl): Only support inline variables
> 	imported from a module.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/cpp1z/inline-var10.C: New test.
> ---
>   gcc/cp/decl.cc                            |  4 ---
>   gcc/cp/decl2.cc                           |  6 +++--
>   gcc/testsuite/g++.dg/cpp1z/inline-var10.C | 33 +++++++++++++++++++++++
>   3 files changed, 37 insertions(+), 6 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/cpp1z/inline-var10.C
> 
> diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc
> index e47f694e4e5..d19d09adde4 100644
> --- a/gcc/cp/decl.cc
> +++ b/gcc/cp/decl.cc
> @@ -7954,10 +7954,6 @@ make_rtl_for_nonlocal_decl (tree decl, tree init, const char* asmspec)
>         && DECL_IMPLICIT_INSTANTIATION (decl))
>       defer_p = 1;
>   
> -  /* Defer vague-linkage variables.  */
> -  if (DECL_INLINE_VAR_P (decl))
> -    defer_p = 1;
> -
>     /* If we're not deferring, go ahead and assemble the variable.  */
>     if (!defer_p)
>       rest_of_decl_compilation (decl, toplev, at_eof);
> diff --git a/gcc/cp/decl2.cc b/gcc/cp/decl2.cc
> index 1dddbaab38b..24a9332ccb1 100644
> --- a/gcc/cp/decl2.cc
> +++ b/gcc/cp/decl2.cc
> @@ -3362,7 +3362,7 @@ import_export_decl (tree decl)
>   
>        * inline functions
>   
> -     * inline variables
> +     * inline variables (from modules)
>   
>        * implicit instantiations of static data members of class
>          templates
> @@ -3385,7 +3385,9 @@ import_export_decl (tree decl)
>   		|| DECL_DECLARED_INLINE_P (decl));
>     else
>       gcc_assert (DECL_IMPLICIT_INSTANTIATION (decl)
> -		|| DECL_INLINE_VAR_P (decl)
> +		|| (DECL_INLINE_VAR_P (decl)
> +		    && DECL_LANG_SPECIFIC (decl)
> +		    && DECL_MODULE_IMPORT_P (decl))
>   		|| DECL_VTABLE_OR_VTT_P (decl)
>   		|| DECL_TINFO_P (decl));
>     /* Check that a definition of DECL is available in this translation
> diff --git a/gcc/testsuite/g++.dg/cpp1z/inline-var10.C b/gcc/testsuite/g++.dg/cpp1z/inline-var10.C
> new file mode 100644
> index 00000000000..8a198556778
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp1z/inline-var10.C
> @@ -0,0 +1,33 @@
> +// PR c++/114013
> +// { dg-do link { target c++17 } }
> +
> +struct S { int a, b; };
> +
> +template <int N>
> +constexpr struct S var[8] = {};
> +
> +template <>
> +constexpr inline struct S var<6>[8] = {
> +  { 1, 1 }, { 2, 0 }, { 3, 1 }, { 4, 0 },
> +  { 5, 1 }, { 6, 0 }, { 7, 1 }, { 8, 0 }
> +};
> +
> +[[gnu::noipa]] void
> +foo (S)
> +{
> +}
> +
> +template <int N>
> +void
> +bar (int x)
> +{
> +  foo (var<N>[x]);
> +}
> +
> +volatile int x;
> +
> +int
> +main ()
> +{
> +  bar <6> (x);
> +}
diff mbox series

Patch

diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc
index e47f694e4e5..d19d09adde4 100644
--- a/gcc/cp/decl.cc
+++ b/gcc/cp/decl.cc
@@ -7954,10 +7954,6 @@  make_rtl_for_nonlocal_decl (tree decl, tree init, const char* asmspec)
       && DECL_IMPLICIT_INSTANTIATION (decl))
     defer_p = 1;
 
-  /* Defer vague-linkage variables.  */
-  if (DECL_INLINE_VAR_P (decl))
-    defer_p = 1;
-
   /* If we're not deferring, go ahead and assemble the variable.  */
   if (!defer_p)
     rest_of_decl_compilation (decl, toplev, at_eof);
diff --git a/gcc/cp/decl2.cc b/gcc/cp/decl2.cc
index 1dddbaab38b..24a9332ccb1 100644
--- a/gcc/cp/decl2.cc
+++ b/gcc/cp/decl2.cc
@@ -3362,7 +3362,7 @@  import_export_decl (tree decl)
 
      * inline functions
 
-     * inline variables
+     * inline variables (from modules)
 
      * implicit instantiations of static data members of class
        templates
@@ -3385,7 +3385,9 @@  import_export_decl (tree decl)
 		|| DECL_DECLARED_INLINE_P (decl));
   else
     gcc_assert (DECL_IMPLICIT_INSTANTIATION (decl)
-		|| DECL_INLINE_VAR_P (decl)
+		|| (DECL_INLINE_VAR_P (decl)
+		    && DECL_LANG_SPECIFIC (decl)
+		    && DECL_MODULE_IMPORT_P (decl))
 		|| DECL_VTABLE_OR_VTT_P (decl)
 		|| DECL_TINFO_P (decl));
   /* Check that a definition of DECL is available in this translation
diff --git a/gcc/testsuite/g++.dg/cpp1z/inline-var10.C b/gcc/testsuite/g++.dg/cpp1z/inline-var10.C
new file mode 100644
index 00000000000..8a198556778
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1z/inline-var10.C
@@ -0,0 +1,33 @@ 
+// PR c++/114013
+// { dg-do link { target c++17 } }
+
+struct S { int a, b; };
+
+template <int N>
+constexpr struct S var[8] = {};
+
+template <>
+constexpr inline struct S var<6>[8] = {
+  { 1, 1 }, { 2, 0 }, { 3, 1 }, { 4, 0 },
+  { 5, 1 }, { 6, 0 }, { 7, 1 }, { 8, 0 }
+};
+
+[[gnu::noipa]] void
+foo (S)
+{
+}
+
+template <int N>
+void
+bar (int x)
+{
+  foo (var<N>[x]);
+}
+
+volatile int x;
+
+int
+main ()
+{
+  bar <6> (x);
+}