diff mbox series

Fix SRA issue with -ftrivial-auto-var-init= [PR121894]

Message ID 20250912190843.4056865-1-qing.zhao@oracle.com
State New
Headers
Series Fix SRA issue with -ftrivial-auto-var-init= [PR121894] |

Commit Message

Qing Zhao Sept. 12, 2025, 7:08 p.m. UTC 
  In tree-sra.cc, for the following stmt (initialization in source code):
s = {};

for the above lhs "s", the field "grp_assignment_write" of the created
struct access is 1;

however, for the following stmt (compiler added initialization):
s = .DEFERRED_INIT (size, init_type, &"s"[0]);

for the above lhs "s", the field "grp_assignment_write" of the created
struct access is 0;

Since the field "grp_assignment_write" of the struct access for the
corresponding LHS "s" is not set correctly when the RHS is .DEFERRED_INIT,
SRA phase didn't do a correct transformation for call to .DEFERRED_INIT.

To fix this issue, we should set the field "grp_assignment_write" correctly
for .DEFERRED_INIT.

	PR 121894

gcc/ChangeLog:

	* tree-sra.cc (scan_function): Set grp_assignment_write to 1 when
	specially handle call to .DEFERRED_INIT.

gcc/testsuite/ChangeLog:

	* g++.dg/auto-init-sra-pr121894.C: New test.

The patch is bootstrapping and regression testing on both x86 and aarch64,
Okay for committing if no issue?

Thanks.

Qing

---
 gcc/testsuite/g++.dg/auto-init-sra-pr121894.C | 22 +++++++++++++++++++
 gcc/tree-sra.cc                               |  8 ++++++-
 2 files changed, 29 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/g++.dg/auto-init-sra-pr121894.C

Comments

Qing Zhao Sept. 15, 2025, 4:39 p.m. UTC | #1 
> On Sep 12, 2025, at 15:08, Qing Zhao <qing.zhao@oracle.com> wrote:
> 
> In tree-sra.cc, for the following stmt (initialization in source code):
> s = {};
> 
> for the above lhs "s", the field "grp_assignment_write" of the created
> struct access is 1;
> 
> however, for the following stmt (compiler added initialization):
> s = .DEFERRED_INIT (size, init_type, &"s"[0]);
> 
> for the above lhs "s", the field "grp_assignment_write" of the created
> struct access is 0;
> 
> Since the field "grp_assignment_write" of the struct access for the
> corresponding LHS "s" is not set correctly when the RHS is .DEFERRED_INIT,
> SRA phase didn't do a correct transformation for call to .DEFERRED_INIT.
> 
> To fix this issue, we should set the field "grp_assignment_write" correctly
> for .DEFERRED_INIT.
> 
> PR 121894
> 
> gcc/ChangeLog:
> 
> * tree-sra.cc (scan_function): Set grp_assignment_write to 1 when
> specially handle call to .DEFERRED_INIT.
> 
> gcc/testsuite/ChangeLog:
> 
> * g++.dg/auto-init-sra-pr121894.C: New test.
> 
> The patch is bootstrapping and regression testing on both x86 and aarch64,

Both bootstrap and regression testing succeed without any issue.

Qing
> Okay for committing if no issue?
> 
> Thanks.
> 
> Qing
> 
> ---
> gcc/testsuite/g++.dg/auto-init-sra-pr121894.C | 22 +++++++++++++++++++
> gcc/tree-sra.cc                               |  8 ++++++-
> 2 files changed, 29 insertions(+), 1 deletion(-)
> create mode 100644 gcc/testsuite/g++.dg/auto-init-sra-pr121894.C
> 
> diff --git a/gcc/testsuite/g++.dg/auto-init-sra-pr121894.C b/gcc/testsuite/g++.dg/auto-init-sra-pr121894.C
> new file mode 100644
> index 00000000000..b1e5a59ffe7
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/auto-init-sra-pr121894.C
> @@ -0,0 +1,22 @@
> +/* Verify that SRA total scalarization will not be confused by padding
> +   and also not confused by auto initialization.  */
> +/* { dg-do compile } */
> +/* { dg-options "-O1 -fdump-tree-esra -ftrivial-auto-var-init=pattern" } */
> +
> +struct S { int a, b, c, d; };
> +void bar (int, int, int, int);
> +
> +void
> +foo ()
> +{
> +  S s;
> +  s.a = 1;
> +  s.c = 2;
> +  s.d = 3;
> +  s.a++;
> +  s.c++;
> +  s.d++;
> +  bar (s.a, s.b, s.c, s.d);
> +}
> +
> +/* { dg-final { scan-tree-dump-times "DEFERRED_INIT" 4 "esra" } } */
> diff --git a/gcc/tree-sra.cc b/gcc/tree-sra.cc
> index 921664ef470..cb0f518ff03 100644
> --- a/gcc/tree-sra.cc
> +++ b/gcc/tree-sra.cc
> @@ -1678,7 +1678,13 @@ scan_function (void)
>  /* If the STMT is a call to DEFERRED_INIT, avoid setting
>     cannot_scalarize_away_bitmap.  */
>  if (gimple_call_internal_p (stmt, IFN_DEFERRED_INIT))
> -    ret |= !!build_access_from_expr_1 (t, stmt, true);
> +    {
> +      struct access *access
> + = build_access_from_expr_1 (t, stmt, true);
> +      if (access)
> + access->grp_assignment_write = 1;
> +      ret |= access != NULL;
> +    }
>  else
>    ret |= build_access_from_expr (t, stmt, true);
> }
> -- 
> 2.31.1
>
Martin Jambor Sept. 17, 2025, 3:59 p.m. UTC | #2 
Hi,

On Fri, Sep 12 2025, Qing Zhao wrote:
> In tree-sra.cc, for the following stmt (initialization in source code):
> s = {};
>
> for the above lhs "s", the field "grp_assignment_write" of the created
> struct access is 1;
>
> however, for the following stmt (compiler added initialization):
> s = .DEFERRED_INIT (size, init_type, &"s"[0]);
>
> for the above lhs "s", the field "grp_assignment_write" of the created
> struct access is 0;
>
> Since the field "grp_assignment_write" of the struct access for the
> corresponding LHS "s" is not set correctly when the RHS is .DEFERRED_INIT,
> SRA phase didn't do a correct transformation for call to .DEFERRED_INIT.
>
> To fix this issue, we should set the field "grp_assignment_write" correctly
> for .DEFERRED_INIT.
>
> 	PR 121894
>
> gcc/ChangeLog:
>
> 	* tree-sra.cc (scan_function): Set grp_assignment_write to 1 when
> 	specially handle call to .DEFERRED_INIT.
>

Thanks,

I cannot approve the change but I think it makes sense.

Martin



> gcc/testsuite/ChangeLog:
>
> 	* g++.dg/auto-init-sra-pr121894.C: New test.
>
> The patch is bootstrapping and regression testing on both x86 and aarch64,
> Okay for committing if no issue?
>
> Thanks.
>
> Qing
>
> ---
>  gcc/testsuite/g++.dg/auto-init-sra-pr121894.C | 22 +++++++++++++++++++
>  gcc/tree-sra.cc                               |  8 ++++++-
>  2 files changed, 29 insertions(+), 1 deletion(-)
>  create mode 100644 gcc/testsuite/g++.dg/auto-init-sra-pr121894.C
>
> diff --git a/gcc/testsuite/g++.dg/auto-init-sra-pr121894.C b/gcc/testsuite/g++.dg/auto-init-sra-pr121894.C
> new file mode 100644
> index 00000000000..b1e5a59ffe7
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/auto-init-sra-pr121894.C
> @@ -0,0 +1,22 @@
> +/* Verify that SRA total scalarization will not be confused by padding
> +   and also not confused by auto initialization.  */
> +/* { dg-do compile } */
> +/* { dg-options "-O1 -fdump-tree-esra -ftrivial-auto-var-init=pattern" } */
> +
> +struct S { int a, b, c, d; };
> +void bar (int, int, int, int);
> +
> +void
> +foo ()
> +{
> +  S s;
> +  s.a = 1;
> +  s.c = 2;
> +  s.d = 3;
> +  s.a++;
> +  s.c++;
> +  s.d++;
> +  bar (s.a, s.b, s.c, s.d);
> +}
> +
> +/* { dg-final { scan-tree-dump-times "DEFERRED_INIT" 4 "esra" } } */
> diff --git a/gcc/tree-sra.cc b/gcc/tree-sra.cc
> index 921664ef470..cb0f518ff03 100644
> --- a/gcc/tree-sra.cc
> +++ b/gcc/tree-sra.cc
> @@ -1678,7 +1678,13 @@ scan_function (void)
>  		  /* If the STMT is a call to DEFERRED_INIT, avoid setting
>  		     cannot_scalarize_away_bitmap.  */
>  		  if (gimple_call_internal_p (stmt, IFN_DEFERRED_INIT))
> -		    ret |= !!build_access_from_expr_1 (t, stmt, true);
> +		    {
> +		      struct access *access
> +			= build_access_from_expr_1 (t, stmt, true);
> +		      if (access)
> +			access->grp_assignment_write = 1;
> +		      ret |= access != NULL;
> +		    }
>  		  else
>  		    ret |= build_access_from_expr (t, stmt, true);
>  		}
> -- 
> 2.31.1
Qing Zhao Sept. 17, 2025, 4:06 p.m. UTC | #3 
> On Sep 17, 2025, at 11:59, Martin Jambor <mjambor@suse.cz> wrote:
> 
> Hi,
> 
> On Fri, Sep 12 2025, Qing Zhao wrote:
>> In tree-sra.cc, for the following stmt (initialization in source code):
>> s = {};
>> 
>> for the above lhs "s", the field "grp_assignment_write" of the created
>> struct access is 1;
>> 
>> however, for the following stmt (compiler added initialization):
>> s = .DEFERRED_INIT (size, init_type, &"s"[0]);
>> 
>> for the above lhs "s", the field "grp_assignment_write" of the created
>> struct access is 0;
>> 
>> Since the field "grp_assignment_write" of the struct access for the
>> corresponding LHS "s" is not set correctly when the RHS is .DEFERRED_INIT,
>> SRA phase didn't do a correct transformation for call to .DEFERRED_INIT.
>> 
>> To fix this issue, we should set the field "grp_assignment_write" correctly
>> for .DEFERRED_INIT.
>> 
>> PR 121894
>> 
>> gcc/ChangeLog:
>> 
>> * tree-sra.cc (scan_function): Set grp_assignment_write to 1 when
>> specially handle call to .DEFERRED_INIT.
>> 
> 
> Thanks,
> 
> I cannot approve the change but I think it makes sense.

Thank you for the review, Martin.

Jakub, could you please also take a look at the change and let me know whether
It’s good for committing?

Thanks.

Qing
> 
> Martin
> 
> 
> 
>> gcc/testsuite/ChangeLog:
>> 
>> * g++.dg/auto-init-sra-pr121894.C: New test.
>> 
>> The patch is bootstrapping and regression testing on both x86 and aarch64,
>> Okay for committing if no issue?
>> 
>> Thanks.
>> 
>> Qing
>> 
>> ---
>> gcc/testsuite/g++.dg/auto-init-sra-pr121894.C | 22 +++++++++++++++++++
>> gcc/tree-sra.cc                               |  8 ++++++-
>> 2 files changed, 29 insertions(+), 1 deletion(-)
>> create mode 100644 gcc/testsuite/g++.dg/auto-init-sra-pr121894.C
>> 
>> diff --git a/gcc/testsuite/g++.dg/auto-init-sra-pr121894.C b/gcc/testsuite/g++.dg/auto-init-sra-pr121894.C
>> new file mode 100644
>> index 00000000000..b1e5a59ffe7
>> --- /dev/null
>> +++ b/gcc/testsuite/g++.dg/auto-init-sra-pr121894.C
>> @@ -0,0 +1,22 @@
>> +/* Verify that SRA total scalarization will not be confused by padding
>> +   and also not confused by auto initialization.  */
>> +/* { dg-do compile } */
>> +/* { dg-options "-O1 -fdump-tree-esra -ftrivial-auto-var-init=pattern" } */
>> +
>> +struct S { int a, b, c, d; };
>> +void bar (int, int, int, int);
>> +
>> +void
>> +foo ()
>> +{
>> +  S s;
>> +  s.a = 1;
>> +  s.c = 2;
>> +  s.d = 3;
>> +  s.a++;
>> +  s.c++;
>> +  s.d++;
>> +  bar (s.a, s.b, s.c, s.d);
>> +}
>> +
>> +/* { dg-final { scan-tree-dump-times "DEFERRED_INIT" 4 "esra" } } */
>> diff --git a/gcc/tree-sra.cc b/gcc/tree-sra.cc
>> index 921664ef470..cb0f518ff03 100644
>> --- a/gcc/tree-sra.cc
>> +++ b/gcc/tree-sra.cc
>> @@ -1678,7 +1678,13 @@ scan_function (void)
>>   /* If the STMT is a call to DEFERRED_INIT, avoid setting
>>      cannot_scalarize_away_bitmap.  */
>>   if (gimple_call_internal_p (stmt, IFN_DEFERRED_INIT))
>> -     ret |= !!build_access_from_expr_1 (t, stmt, true);
>> +     {
>> +       struct access *access
>> + = build_access_from_expr_1 (t, stmt, true);
>> +       if (access)
>> + access->grp_assignment_write = 1;
>> +       ret |= access != NULL;
>> +     }
>>   else
>>     ret |= build_access_from_expr (t, stmt, true);
>> }
>> -- 
>> 2.31.1
Jakub Jelinek Sept. 17, 2025, 4:11 p.m. UTC | #4 
On Wed, Sep 17, 2025 at 05:59:34PM +0200, Martin Jambor wrote:
> Hi,
> 
> On Fri, Sep 12 2025, Qing Zhao wrote:
> > In tree-sra.cc, for the following stmt (initialization in source code):
> > s = {};
> >
> > for the above lhs "s", the field "grp_assignment_write" of the created
> > struct access is 1;
> >
> > however, for the following stmt (compiler added initialization):
> > s = .DEFERRED_INIT (size, init_type, &"s"[0]);
> >
> > for the above lhs "s", the field "grp_assignment_write" of the created
> > struct access is 0;
> >
> > Since the field "grp_assignment_write" of the struct access for the
> > corresponding LHS "s" is not set correctly when the RHS is .DEFERRED_INIT,
> > SRA phase didn't do a correct transformation for call to .DEFERRED_INIT.
> >
> > To fix this issue, we should set the field "grp_assignment_write" correctly
> > for .DEFERRED_INIT.
> >
> > 	PR 121894

This should be
	PR tree-optimization/121894

> >
> > gcc/ChangeLog:
> >
> > 	* tree-sra.cc (scan_function): Set grp_assignment_write to 1 when
> > 	specially handle call to .DEFERRED_INIT.
> 
> I cannot approve the change but I think it makes sense.

LGTM then with nits.

> > gcc/testsuite/ChangeLog:
> >
> > 	* g++.dg/auto-init-sra-pr121894.C: New test.

Please move the test into g++.dg/opt/ directory or g++.dg/tree-ssa/.
g++.dg/ ideally shouldn't have any tests directly in it.

	Jakub
Qing Zhao Sept. 17, 2025, 5 p.m. UTC | #5 
> On Sep 17, 2025, at 12:11, Jakub Jelinek <jakub@redhat.com> wrote:
> 
> On Wed, Sep 17, 2025 at 05:59:34PM +0200, Martin Jambor wrote:
>> Hi,
>> 
>> On Fri, Sep 12 2025, Qing Zhao wrote:
>>> In tree-sra.cc, for the following stmt (initialization in source code):
>>> s = {};
>>> 
>>> for the above lhs "s", the field "grp_assignment_write" of the created
>>> struct access is 1;
>>> 
>>> however, for the following stmt (compiler added initialization):
>>> s = .DEFERRED_INIT (size, init_type, &"s"[0]);
>>> 
>>> for the above lhs "s", the field "grp_assignment_write" of the created
>>> struct access is 0;
>>> 
>>> Since the field "grp_assignment_write" of the struct access for the
>>> corresponding LHS "s" is not set correctly when the RHS is .DEFERRED_INIT,
>>> SRA phase didn't do a correct transformation for call to .DEFERRED_INIT.
>>> 
>>> To fix this issue, we should set the field "grp_assignment_write" correctly
>>> for .DEFERRED_INIT.
>>> 
>>> PR 121894
> 
> This should be
> PR tree-optimization/121894

Okay.
> 
>>> 
>>> gcc/ChangeLog:
>>> 
>>> * tree-sra.cc (scan_function): Set grp_assignment_write to 1 when
>>> specially handle call to .DEFERRED_INIT.
>> 
>> I cannot approve the change but I think it makes sense.
> 
> LGTM then with nits.
> 
>>> gcc/testsuite/ChangeLog:
>>> 
>>> * g++.dg/auto-init-sra-pr121894.C: New test.
> 
> Please move the test into g++.dg/opt/ directory or g++.dg/tree-ssa/.
> g++.dg/ ideally shouldn't have any tests directly in it.

Sure, will do that before committing.

thanks.

Qing
> 
> Jakub
diff mbox series

Patch

diff --git a/gcc/testsuite/g++.dg/auto-init-sra-pr121894.C b/gcc/testsuite/g++.dg/auto-init-sra-pr121894.C
new file mode 100644
index 00000000000..b1e5a59ffe7
--- /dev/null
+++ b/gcc/testsuite/g++.dg/auto-init-sra-pr121894.C
@@ -0,0 +1,22 @@ 
+/* Verify that SRA total scalarization will not be confused by padding
+   and also not confused by auto initialization.  */
+/* { dg-do compile } */
+/* { dg-options "-O1 -fdump-tree-esra -ftrivial-auto-var-init=pattern" } */
+
+struct S { int a, b, c, d; };
+void bar (int, int, int, int);
+
+void
+foo ()
+{
+  S s;
+  s.a = 1;
+  s.c = 2;
+  s.d = 3;
+  s.a++;
+  s.c++;
+  s.d++;
+  bar (s.a, s.b, s.c, s.d);
+}
+
+/* { dg-final { scan-tree-dump-times "DEFERRED_INIT" 4 "esra" } } */
diff --git a/gcc/tree-sra.cc b/gcc/tree-sra.cc
index 921664ef470..cb0f518ff03 100644
--- a/gcc/tree-sra.cc
+++ b/gcc/tree-sra.cc
@@ -1678,7 +1678,13 @@  scan_function (void)
 		  /* If the STMT is a call to DEFERRED_INIT, avoid setting
 		     cannot_scalarize_away_bitmap.  */
 		  if (gimple_call_internal_p (stmt, IFN_DEFERRED_INIT))
-		    ret |= !!build_access_from_expr_1 (t, stmt, true);
+		    {
+		      struct access *access
+			= build_access_from_expr_1 (t, stmt, true);
+		      if (access)
+			access->grp_assignment_write = 1;
+		      ret |= access != NULL;
+		    }
 		  else
 		    ret |= build_access_from_expr (t, stmt, true);
 		}