diff mbox series

middle-end: don't cache restart_loop in vectorizable_live_operations [PR113808]

Message ID patch-18251-tamar@arm.com
State Not Applicable
Headers
Series middle-end: don't cache restart_loop in vectorizable_live_operations [PR113808] |

Checks

Context Check Description
snail/gcc-patch-check fail Git am fail log

Commit Message

Tamar Christina Feb. 8, 2024, 10:37 a.m. UTC 
  Hi All,

There's a bug in vectorizable_live_operation that restart_loop is defined
outside the loop.

This variable is supposed to indicate whether we are doing a first or last
index reduction.  The problem is that by defining it outside the loop it becomes
dependent on the order we visit the USE/DEFs.

In the given example, the loop isn't PEELED, but we visit the early exit uses
first.  This then sets the boolean to true and it can't get to false again.

So when we visit the main exit we still treat it as an early exit for that
SSA name.

This cleans it up and renames the variables to something that's hopefully
clearer to their intention.

Bootstrapped Regtested on aarch64-none-linux-gnu and
x86_64-pc-linux-gnu no issues.

Ok for master?

Thanks,
Tamar

gcc/ChangeLog:

	PR tree-optimization/113808
	* tree-vect-loop.cc (vectorizable_live_operation): Don't cache the 
	value cross iterations.

gcc/testsuite/ChangeLog:

	PR tree-optimization/113808
	* gfortran.dg/vect/vect-early-break_1-PR113808.f90: New test.

--- inline copy of patch -- 
diff --git a/gcc/testsuite/gfortran.dg/vect/vect-early-break_1-PR113808.f90 b/gcc/testsuite/gfortran.dg/vect/vect-early-break_1-PR113808.f90
new file mode 100644
index 0000000000000000000000000000000000000000..5c339fa7a348fac5527bbbf456a535da96b5c1ed




--
diff --git a/gcc/testsuite/gfortran.dg/vect/vect-early-break_1-PR113808.f90 b/gcc/testsuite/gfortran.dg/vect/vect-early-break_1-PR113808.f90
new file mode 100644
index 0000000000000000000000000000000000000000..5c339fa7a348fac5527bbbf456a535da96b5c1ed
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/vect/vect-early-break_1-PR113808.f90
@@ -0,0 +1,21 @@
+! { dg-add-options vect_early_break }
+! { dg-require-effective-target vect_early_break }
+! { dg-require-effective-target vect_long_long }
+! { dg-additional-options "-fopenmp-simd" }
+
+! { dg-final { scan-tree-dump "LOOP VECTORIZED" "vect" } }
+
+program main
+  integer :: n, i,k
+  n = 11
+  do i = 1, n,2
+    !$omp simd lastprivate(k)
+    do k = 1, i + 41
+      if (k > 11 + 41 .or. k < 1) error stop
+    end do
+  end do
+  if (k /= 53) then
+    print *, k, 53
+    error stop
+  endif
+end
diff --git a/gcc/tree-vect-loop.cc b/gcc/tree-vect-loop.cc
index 190df9ec7741fd05aa0b9abe150baf06b2ca9a57..eed2268e9bae7e7ad36d13da03e0b54eab26ef6f 100644
--- a/gcc/tree-vect-loop.cc
+++ b/gcc/tree-vect-loop.cc
@@ -10950,7 +10950,7 @@ vectorizable_live_operation (vec_info *vinfo, stmt_vec_info stmt_info,
 	 did.  For the live values we want the value at the start of the iteration
 	 rather than at the end.  */
       edge main_e = LOOP_VINFO_IV_EXIT (loop_vinfo);
-      bool restart_loop = LOOP_VINFO_EARLY_BREAKS_VECT_PEELED (loop_vinfo);
+      bool all_exits_as_early_p = LOOP_VINFO_EARLY_BREAKS_VECT_PEELED (loop_vinfo);
       FOR_EACH_IMM_USE_STMT (use_stmt, imm_iter, lhs)
 	if (!is_gimple_debug (use_stmt)
 	    && !flow_bb_inside_loop_p (loop, gimple_bb (use_stmt)))
@@ -10966,8 +10966,7 @@ vectorizable_live_operation (vec_info *vinfo, stmt_vec_info stmt_info,
 	      /* For early exit where the exit is not in the BB that leads
 		 to the latch then we're restarting the iteration in the
 		 scalar loop.  So get the first live value.  */
-	      restart_loop = restart_loop || !main_exit_edge;
-	      if (restart_loop
+	      if ((all_exits_as_early_p || !main_exit_edge)
 		  && STMT_VINFO_DEF_TYPE (stmt_info) == vect_induction_def)
 		{
 		  tmp_vec_lhs = vec_lhs0;

Comments

Richard Biener Feb. 8, 2024, 10:40 a.m. UTC | #1 
On Thu, 8 Feb 2024, Tamar Christina wrote:

> Hi All,
> 
> There's a bug in vectorizable_live_operation that restart_loop is defined
> outside the loop.
> 
> This variable is supposed to indicate whether we are doing a first or last
> index reduction.  The problem is that by defining it outside the loop it becomes
> dependent on the order we visit the USE/DEFs.
> 
> In the given example, the loop isn't PEELED, but we visit the early exit uses
> first.  This then sets the boolean to true and it can't get to false again.
> 
> So when we visit the main exit we still treat it as an early exit for that
> SSA name.
> 
> This cleans it up and renames the variables to something that's hopefully
> clearer to their intention.
> 
> Bootstrapped Regtested on aarch64-none-linux-gnu and
> x86_64-pc-linux-gnu no issues.
> 
> Ok for master?

OK.

Thanks,
Richard.

> Thanks,
> Tamar
> 
> gcc/ChangeLog:
> 
> 	PR tree-optimization/113808
> 	* tree-vect-loop.cc (vectorizable_live_operation): Don't cache the 
> 	value cross iterations.
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR tree-optimization/113808
> 	* gfortran.dg/vect/vect-early-break_1-PR113808.f90: New test.
> 
> --- inline copy of patch -- 
> diff --git a/gcc/testsuite/gfortran.dg/vect/vect-early-break_1-PR113808.f90 b/gcc/testsuite/gfortran.dg/vect/vect-early-break_1-PR113808.f90
> new file mode 100644
> index 0000000000000000000000000000000000000000..5c339fa7a348fac5527bbbf456a535da96b5c1ed
> --- /dev/null
> +++ b/gcc/testsuite/gfortran.dg/vect/vect-early-break_1-PR113808.f90
> @@ -0,0 +1,21 @@
> +! { dg-add-options vect_early_break }
> +! { dg-require-effective-target vect_early_break }
> +! { dg-require-effective-target vect_long_long }
> +! { dg-additional-options "-fopenmp-simd" }
> +
> +! { dg-final { scan-tree-dump "LOOP VECTORIZED" "vect" } }
> +
> +program main
> +  integer :: n, i,k
> +  n = 11
> +  do i = 1, n,2
> +    !$omp simd lastprivate(k)
> +    do k = 1, i + 41
> +      if (k > 11 + 41 .or. k < 1) error stop
> +    end do
> +  end do
> +  if (k /= 53) then
> +    print *, k, 53
> +    error stop
> +  endif
> +end
> diff --git a/gcc/tree-vect-loop.cc b/gcc/tree-vect-loop.cc
> index 190df9ec7741fd05aa0b9abe150baf06b2ca9a57..eed2268e9bae7e7ad36d13da03e0b54eab26ef6f 100644
> --- a/gcc/tree-vect-loop.cc
> +++ b/gcc/tree-vect-loop.cc
> @@ -10950,7 +10950,7 @@ vectorizable_live_operation (vec_info *vinfo, stmt_vec_info stmt_info,
>  	 did.  For the live values we want the value at the start of the iteration
>  	 rather than at the end.  */
>        edge main_e = LOOP_VINFO_IV_EXIT (loop_vinfo);
> -      bool restart_loop = LOOP_VINFO_EARLY_BREAKS_VECT_PEELED (loop_vinfo);
> +      bool all_exits_as_early_p = LOOP_VINFO_EARLY_BREAKS_VECT_PEELED (loop_vinfo);
>        FOR_EACH_IMM_USE_STMT (use_stmt, imm_iter, lhs)
>  	if (!is_gimple_debug (use_stmt)
>  	    && !flow_bb_inside_loop_p (loop, gimple_bb (use_stmt)))
> @@ -10966,8 +10966,7 @@ vectorizable_live_operation (vec_info *vinfo, stmt_vec_info stmt_info,
>  	      /* For early exit where the exit is not in the BB that leads
>  		 to the latch then we're restarting the iteration in the
>  		 scalar loop.  So get the first live value.  */
> -	      restart_loop = restart_loop || !main_exit_edge;
> -	      if (restart_loop
> +	      if ((all_exits_as_early_p || !main_exit_edge)
>  		  && STMT_VINFO_DEF_TYPE (stmt_info) == vect_induction_def)
>  		{
>  		  tmp_vec_lhs = vec_lhs0;
> 
> 
> 
> 
>
Jakub Jelinek Feb. 8, 2024, 10:45 a.m. UTC | #2 
On Thu, Feb 08, 2024 at 10:37:14AM +0000, Tamar Christina wrote:
> 	PR tree-optimization/113808
> 	* tree-vect-loop.cc (vectorizable_live_operation): Don't cache the 
> 	value cross iterations.
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR tree-optimization/113808
> 	* gfortran.dg/vect/vect-early-break_1-PR113808.f90: New test.

Please use lowercase pr instead of PR for consistency with all the other
tests in the directory.

> --- inline copy of patch -- 
> diff --git a/gcc/testsuite/gfortran.dg/vect/vect-early-break_1-PR113808.f90 b/gcc/testsuite/gfortran.dg/vect/vect-early-break_1-PR113808.f90
> new file mode 100644
> index 0000000000000000000000000000000000000000..5c339fa7a348fac5527bbbf456a535da96b5c1ed
> --- /dev/null
> +++ b/gcc/testsuite/gfortran.dg/vect/vect-early-break_1-PR113808.f90
> @@ -0,0 +1,21 @@
> +! { dg-add-options vect_early_break }
> +! { dg-require-effective-target vect_early_break }
> +! { dg-require-effective-target vect_long_long }
> +! { dg-additional-options "-fopenmp-simd" }
> +
> +! { dg-final { scan-tree-dump "LOOP VECTORIZED" "vect" } }
> +
> +program main
> +  integer :: n, i,k
> +  n = 11
> +  do i = 1, n,2
> +    !$omp simd lastprivate(k)

Please either drop lastprivate(k) clause or use linear(k:1)
The iteration var of simd loop without collapse or with
collapse(1) is implicitly linear with the step, and even linear
means the value from the last iteration can be used after the
simd construct.  Overriding the data sharing to something different
has been only added recently to OpenMP and isn't really needed here.

> +    do k = 1, i + 41
> +      if (k > 11 + 41 .or. k < 1) error stop
> +    end do
> +  end do
> +  if (k /= 53) then
> +    print *, k, 53
> +    error stop
> +  endif
> +end

	Jakub
Tamar Christina Feb. 8, 2024, 10:54 a.m. UTC | #3 
> Please either drop lastprivate(k) clause or use linear(k:1)
> The iteration var of simd loop without collapse or with
> collapse(1) is implicitly linear with the step, and even linear
> means the value from the last iteration can be used after the
> simd construct.  Overriding the data sharing to something different
> has been only added recently to OpenMP and isn't really needed here.
> 

Sorry I know very little about fortran, is this ok?

Regtested on aarch64-none-linux-gnu and no issues.

Ok for master?

Thanks,
Tamar

gcc/testsuite/ChangeLog:

	PR tree-optimization/113808
	* gfortran.dg/vect/vect-early-break_1-PR113808.f90: Moved to...
	* gfortran.dg/vect/vect-early-break_1-pr113808.f90: ...here.

--- inline copy of patch ---

diff --git a/gcc/testsuite/gfortran.dg/vect/vect-early-break_1-PR113808.f90 b/gcc/testsuite/gfortran.dg/vect/vect-early-break_1-pr113808.f90
similarity index 93%
rename from gcc/testsuite/gfortran.dg/vect/vect-early-break_1-PR113808.f90
rename to gcc/testsuite/gfortran.dg/vect/vect-early-break_1-pr113808.f90
index 5c339fa7a348fac5527bbbf456a535da96b5c1ed..6f92e9095bdee08a5a9db2816f57da6c14d91b11 100644
--- a/gcc/testsuite/gfortran.dg/vect/vect-early-break_1-PR113808.f90
+++ b/gcc/testsuite/gfortran.dg/vect/vect-early-break_1-pr113808.f90
@@ -9,7 +9,7 @@ program main
   integer :: n, i,k
   n = 11
   do i = 1, n,2
-    !$omp simd lastprivate(k)
+    !$omp simd
     do k = 1, i + 41
       if (k > 11 + 41 .or. k < 1) error stop
     end do
Jakub Jelinek Feb. 8, 2024, 10:56 a.m. UTC | #4 
On Thu, Feb 08, 2024 at 10:54:07AM +0000, Tamar Christina wrote:
> > Please either drop lastprivate(k) clause or use linear(k:1)
> > The iteration var of simd loop without collapse or with
> > collapse(1) is implicitly linear with the step, and even linear
> > means the value from the last iteration can be used after the
> > simd construct.  Overriding the data sharing to something different
> > has been only added recently to OpenMP and isn't really needed here.
> > 
> 
> Sorry I know very little about fortran, is this ok?
> 
> Regtested on aarch64-none-linux-gnu and no issues.
> 
> Ok for master?
> 
> Thanks,
> Tamar
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR tree-optimization/113808
> 	* gfortran.dg/vect/vect-early-break_1-PR113808.f90: Moved to...
> 	* gfortran.dg/vect/vect-early-break_1-pr113808.f90: ...here.

Yes, thanks.

	Jakub
diff mbox series

Patch

--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/vect/vect-early-break_1-PR113808.f90
@@ -0,0 +1,21 @@ 
+! { dg-add-options vect_early_break }
+! { dg-require-effective-target vect_early_break }
+! { dg-require-effective-target vect_long_long }
+! { dg-additional-options "-fopenmp-simd" }
+
+! { dg-final { scan-tree-dump "LOOP VECTORIZED" "vect" } }
+
+program main
+  integer :: n, i,k
+  n = 11
+  do i = 1, n,2
+    !$omp simd lastprivate(k)
+    do k = 1, i + 41
+      if (k > 11 + 41 .or. k < 1) error stop
+    end do
+  end do
+  if (k /= 53) then
+    print *, k, 53
+    error stop
+  endif
+end
diff --git a/gcc/tree-vect-loop.cc b/gcc/tree-vect-loop.cc
index 190df9ec7741fd05aa0b9abe150baf06b2ca9a57..eed2268e9bae7e7ad36d13da03e0b54eab26ef6f 100644
--- a/gcc/tree-vect-loop.cc
+++ b/gcc/tree-vect-loop.cc
@@ -10950,7 +10950,7 @@  vectorizable_live_operation (vec_info *vinfo, stmt_vec_info stmt_info,
 	 did.  For the live values we want the value at the start of the iteration
 	 rather than at the end.  */
       edge main_e = LOOP_VINFO_IV_EXIT (loop_vinfo);
-      bool restart_loop = LOOP_VINFO_EARLY_BREAKS_VECT_PEELED (loop_vinfo);
+      bool all_exits_as_early_p = LOOP_VINFO_EARLY_BREAKS_VECT_PEELED (loop_vinfo);
       FOR_EACH_IMM_USE_STMT (use_stmt, imm_iter, lhs)
 	if (!is_gimple_debug (use_stmt)
 	    && !flow_bb_inside_loop_p (loop, gimple_bb (use_stmt)))
@@ -10966,8 +10966,7 @@  vectorizable_live_operation (vec_info *vinfo, stmt_vec_info stmt_info,
 	      /* For early exit where the exit is not in the BB that leads
 		 to the latch then we're restarting the iteration in the
 		 scalar loop.  So get the first live value.  */
-	      restart_loop = restart_loop || !main_exit_edge;
-	      if (restart_loop
+	      if ((all_exits_as_early_p || !main_exit_edge)
 		  && STMT_VINFO_DEF_TYPE (stmt_info) == vect_induction_def)
 		{
 		  tmp_vec_lhs = vec_lhs0;