[PATCH] OpenMP: warn about iteration var modifications in loop body

[PATCH] OpenMP: warn about iteration var modifications in loop body

[Frederik Harwath]

[-- Attachment #1: Type: text/plain, Size: 960 bytes --]

Hi,

this patch implements a warning about (some simple cases of direct)
modifications of iteration variables in OpenMP loops which are forbidden
according to the OpenMP specification. I think this can be helpful,
especially for new OpenMP users. I have implemented this after I
observed some confusion concerning this topic recently.
The check is implemented during gimplification. It reuses the
"loop_iter_var" vector in the "gimplify_omp_ctx" which was previously
only used for "doacross" handling to identify the loop iteration
variables during the gimplification of MODIFY_EXPRs in omp_for bodies.
I have only added a common C/C++ test because I don't see any special
C++ constructs for which a warning *should* be emitted and Fortran
rejects modifications of iteration variables in do loops in general.

I have run "make check" on x86_64-linux-gnu and not observed any
regressions.

Is it ok to commit this?

Best regards,
Frederik

[-- Attachment #2: 0001-OpenMP-warn-about-iteration-var-modifications-in-loo.patch --]
[-- Type: text/x-patch, Size: 9556 bytes --]

From 4944a9f94bcda9907e0118e71137ee7e192657c2 Mon Sep 17 00:00:00 2001
From: Frederik Harwath <frederik@harwath.name>
Date: Tue, 27 Feb 2024 21:07:00 +0000
Subject: [PATCH] OpenMP: warn about iteration var modifications in loop body

OpenMP loop iteration variables may not be changed by user code in the
loop body according to the OpenMP specification.  In general, the
compiler cannot enforce this, but nevertheless simple cases in which
the user modifies the iteration variable directly in the loop body
(in contrast to, e.g., modifications through a pointer) can be recognized. A
warning should be useful, for instance, to new users of OpenMP.

This commit implements a warning about forbidden iteration var modifications
during gimplification. It reuses the "loop_iter_var" vector in the
"gimplify_omp_ctx" which was previously only used for "doacross" handling to
identify the loop iteration variables during the gimplification of MODIFY_EXPRs
in omp_for bodies.

gcc/ChangeLog:

	* gimplify.cc (struct gimplify_omp_ctx): Add field "in_omp_for_body" to
	recognize the gimplification state during which the new warning should
	be emitted. Add field "is_doacross" to distinguish the original use of
	"loop_iter_var" from its new use.
	(new_omp_context): Initialize new gimplify_omp_ctx fields.
	(gimplify_modify_expr): Emit warning if iter var is modified.
	(gimplify_omp_for): Make initialization and filling of loop_iter_var
	vector unconditional and adjust new gimplify_omp_ctx fields before
	gimplifying the omp_for body.
	(gimplify_omp_ordered): Check for do_across field in addition to
	emptiness check on loop_iter_var vector since the vector is now always
	being filled.

gcc/testsuite/ChangeLog:

	* c-c++-common/gomp/iter-var-modification.c: New test.

Signed-off-by: Frederik Harwath  <frederik@harwath.name>
---
 gcc/gimplify.cc                               |  54 +++++++---
 .../c-c++-common/gomp/iter-var-modification.c | 100 ++++++++++++++++++
 2 files changed, 138 insertions(+), 16 deletions(-)
 create mode 100644 gcc/testsuite/c-c++-common/gomp/iter-var-modification.c

diff --git a/gcc/gimplify.cc b/gcc/gimplify.cc
index 7f79b3cc7e6..a74ad987cf7 100644
--- a/gcc/gimplify.cc
+++ b/gcc/gimplify.cc
@@ -235,6 +235,8 @@ struct gimplify_omp_ctx
   bool order_concurrent;
   bool has_depend;
   bool in_for_exprs;
+  bool in_omp_for_body;
+  bool is_doacross;
   int defaultmap[5];
 };
 
@@ -456,6 +458,10 @@ new_omp_context (enum omp_region_type region_type)
   c->privatized_types = new hash_set<tree>;
   c->location = input_location;
   c->region_type = region_type;
+  c->loop_iter_var.create (0);
+  c->in_omp_for_body = false;
+  c->is_doacross = false;
+
   if ((region_type & ORT_TASK) == 0)
     c->default_kind = OMP_CLAUSE_DEFAULT_SHARED;
   else
@@ -6312,6 +6318,18 @@ gimplify_modify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
   gcc_assert (TREE_CODE (*expr_p) == MODIFY_EXPR
 	      || TREE_CODE (*expr_p) == INIT_EXPR);
 
+  if (gimplify_omp_ctxp && gimplify_omp_ctxp->in_omp_for_body)
+    {
+      size_t num_vars = gimplify_omp_ctxp->loop_iter_var.length () / 2;
+      for (size_t i = 0; i < num_vars; i++)
+	{
+	  if (*to_p == gimplify_omp_ctxp->loop_iter_var[2 * i + 1])
+	    warning_at (input_location, OPT_Wopenmp,
+			"forbidden modification of iteration variable %qE in "
+			"OpenMP loop", *to_p);
+	}
+    }
+
   /* Trying to simplify a clobber using normal logic doesn't work,
      so handle it here.  */
   if (TREE_CLOBBER_P (*from_p))
@@ -15334,6 +15352,8 @@ gimplify_omp_for (tree *expr_p, gimple_seq *pre_p)
 	      == TREE_VEC_LENGTH (OMP_FOR_COND (for_stmt)));
   gcc_assert (TREE_VEC_LENGTH (OMP_FOR_INIT (for_stmt))
 	      == TREE_VEC_LENGTH (OMP_FOR_INCR (for_stmt)));
+  int len = TREE_VEC_LENGTH (OMP_FOR_INIT (for_stmt));
+  gimplify_omp_ctxp->loop_iter_var.create (len * 2);
 
   tree c = omp_find_clause (OMP_FOR_CLAUSES (for_stmt), OMP_CLAUSE_ORDERED);
   bool is_doacross = false;
@@ -15342,8 +15362,6 @@ gimplify_omp_for (tree *expr_p, gimple_seq *pre_p)
     {
       OMP_CLAUSE_ORDERED_DOACROSS (c) = 1;
       is_doacross = true;
-      int len = TREE_VEC_LENGTH (OMP_FOR_INIT (for_stmt));
-      gimplify_omp_ctxp->loop_iter_var.create (len * 2);
       for (tree *pc = &OMP_FOR_CLAUSES (for_stmt); *pc; )
 	if (OMP_CLAUSE_CODE (*pc) == OMP_CLAUSE_LINEAR)
 	  {
@@ -15380,23 +15398,22 @@ gimplify_omp_for (tree *expr_p, gimple_seq *pre_p)
       gcc_assert (DECL_P (decl));
       gcc_assert (INTEGRAL_TYPE_P (TREE_TYPE (decl))
 		  || POINTER_TYPE_P (TREE_TYPE (decl)));
-      if (is_doacross)
+
+      if (TREE_CODE (for_stmt) == OMP_FOR && OMP_FOR_ORIG_DECLS (for_stmt))
 	{
-	  if (TREE_CODE (for_stmt) == OMP_FOR && OMP_FOR_ORIG_DECLS (for_stmt))
+	  tree orig_decl = TREE_VEC_ELT (OMP_FOR_ORIG_DECLS (for_stmt), i);
+	  if (TREE_CODE (orig_decl) == TREE_LIST)
 	    {
-	      tree orig_decl = TREE_VEC_ELT (OMP_FOR_ORIG_DECLS (for_stmt), i);
-	      if (TREE_CODE (orig_decl) == TREE_LIST)
-		{
-		  orig_decl = TREE_PURPOSE (orig_decl);
-		  if (!orig_decl)
-		    orig_decl = decl;
-		}
-	      gimplify_omp_ctxp->loop_iter_var.quick_push (orig_decl);
+	      orig_decl = TREE_PURPOSE (orig_decl);
+	      if (!orig_decl)
+		orig_decl = decl;
 	    }
-	  else
-	    gimplify_omp_ctxp->loop_iter_var.quick_push (decl);
-	  gimplify_omp_ctxp->loop_iter_var.quick_push (decl);
+	  gimplify_omp_ctxp->loop_iter_var.quick_push (orig_decl);
 	}
+      else
+	gimplify_omp_ctxp->loop_iter_var.quick_push (decl);
+      gimplify_omp_ctxp->loop_iter_var.quick_push (decl);
+
 
       if (for_stmt == orig_for_stmt)
 	{
@@ -15818,9 +15835,13 @@ gimplify_omp_for (tree *expr_p, gimple_seq *pre_p)
 	  TREE_SIDE_EFFECTS (OMP_FOR_BODY (orig_for_stmt)) = 1;
 	}
     }
+  gimplify_omp_ctxp->in_omp_for_body = true;
+  gimplify_omp_ctxp->is_doacross = is_doacross;
 
   gimple *g = gimplify_and_return_first (OMP_FOR_BODY (orig_for_stmt),
 					 &for_body);
+  gimplify_omp_ctxp->in_omp_for_body = false;
+  gimplify_omp_ctxp->is_doacross = false;
 
   if (TREE_CODE (orig_for_stmt) == OMP_TASKLOOP
       || (loop_p && orig_for_stmt == for_stmt))
@@ -17430,7 +17451,8 @@ gimplify_omp_ordered (tree expr, gimple_seq body)
     {
       for (c = OMP_ORDERED_CLAUSES (expr); c; c = OMP_CLAUSE_CHAIN (c))
 	if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_DOACROSS
-	    && gimplify_omp_ctxp->loop_iter_var.is_empty ())
+	    && (!gimplify_omp_ctxp->is_doacross
+		|| gimplify_omp_ctxp->loop_iter_var.is_empty ()))
 	  {
 	    error_at (OMP_CLAUSE_LOCATION (c),
 		      "%<ordered%> construct with %qs clause must be "
diff --git a/gcc/testsuite/c-c++-common/gomp/iter-var-modification.c b/gcc/testsuite/c-c++-common/gomp/iter-var-modification.c
new file mode 100644
index 00000000000..5662fce2a6f
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/gomp/iter-var-modification.c
@@ -0,0 +1,100 @@
+extern int a[1000];
+
+int main ()
+{
+#pragma omp for
+  for (int i = 0; i < 1000; i++)
+    {
+      if (i % 2  == 0)
+	i++; /* { dg-warning {forbidden modification of iteration variable .i. in OpenMP loop} } */
+    }
+
+  #pragma omp for
+  for (int i = 0; i < 1000; i++)
+    {
+      if (i % 2  == 0);
+      else
+	i++; /* { dg-warning {forbidden modification of iteration variable .i. in OpenMP loop} } */
+    }
+
+#pragma omp for
+  for (int i = 0; i < 1000; i++)
+    {
+	i = 0; /* { dg-warning {forbidden modification of iteration variable .i. in OpenMP loop} } */
+    }
+
+#pragma omp for
+  for (int i = 0; i != 1000; i++)
+    {
+	i = 0; /* { dg-warning {forbidden modification of iteration variable .i. in OpenMP loop} } */
+    }
+
+#pragma omp for
+  for (int i = 1000; i > 0; i--)
+    {
+	i = 0; /* { dg-warning {forbidden modification of iteration variable .i. in OpenMP loop} } */
+    }
+
+#pragma omp for
+  for (int *p = (int*)&a; p < a + 1000; p++)
+    {
+      p = (int*)&a; /* { dg-warning {forbidden modification of iteration variable .p. in OpenMP loop} } */
+    }
+
+#pragma omp for
+  for (int *p = (int*)&a; p < a + 1000; p++)
+    {
+	*p = 0;
+    }
+
+#pragma omp parallel for collapse(3)
+  for (int i = 0; i < 1000; i++)
+      for (int j = 0; j < 1000; j++)
+	for (int k = 0; k < 1000; k++)
+
+    {
+	j++; /* { dg-warning {forbidden modification of iteration variable .j. in OpenMP loop} } */
+    }
+
+#pragma omp target teams distribute parallel for collapse(3)
+  for (int i = 0; i < 1000; i++)
+      for (int j = 0; j < 1000; j++)
+	for (int k = 0; k < 1000; k++)
+
+    {
+	k++; /* { dg-warning {forbidden modification of iteration variable .k. in OpenMP loop} } */
+    }
+
+#pragma omp parallel for collapse(2)
+  for (int i = 0; i < 1000; i++)
+      for (int j = 0; j < 1000; j++)
+	for (int k = 0; k < 1000; k++)
+
+    {
+	j++; /* { dg-warning {forbidden modification of iteration variable .j. in OpenMP loop} } */
+    }
+
+#pragma omp target teams distribute parallel for collapse(2)
+  for (int i = 0; i < 1000; i++)
+      for (int j = 0; j < 1000; j++)
+	for (int k = 0; k < 1000; k++)
+
+    {
+	k++; /* No error since third loop is not included in collapsed loop-nest. */
+    }
+
+  /* Only modifications in the body should be reported.  Do not warn about
+     assignments to i,k in the pre-body. */
+  int i = 0;
+  int k = 0;
+#pragma omp target teams distribute parallel for collapse(3)
+  for (i = 1; i < 1000; i++)
+      for (int j = 0; j < 1000; j++)
+	for (k = 1; k < 1000; k++)
+
+    {
+	j++; /* { dg-warning {forbidden modification of iteration variable .j. in OpenMP loop} } */
+    }
+
+  return 0;
+}
-- 
2.34.1

Re: [PATCH] OpenMP: warn about iteration var modifications in loop body

[Frederik Harwath]

[-- Attachment #1: Type: text/plain, Size: 1235 bytes --]

Ping.


The Linaro CI has kindly pointed me to two test regressions that I had
missed. I have adjust the test expectations in the updated patch which I
have attached.

Frederik


On 28.02.24 8:32 PM, Frederik Harwath wrote:
> Hi,
>
> this patch implements a warning about (some simple cases of direct)
> modifications of iteration variables in OpenMP loops which are
> forbidden according to the OpenMP specification. I think this can be
> helpful, especially for new OpenMP users. I have implemented this
> after I observed some confusion concerning this topic recently.
> The check is implemented during gimplification. It reuses the
> "loop_iter_var" vector in the "gimplify_omp_ctx" which was previously
> only used for "doacross" handling to identify the loop iteration
> variables during the gimplification of MODIFY_EXPRs in omp_for bodies.
> I have only added a common C/C++ test because I don't see any special
> C++ constructs for which a warning *should* be emitted and Fortran
> rejects modifications of iteration variables in do loops in general.
>
> I have run "make check" on x86_64-linux-gnu and not observed any
> regressions.
>
> Is it ok to commit this?
>
> Best regards,
> Frederik

[-- Attachment #2: 0001-OpenMP-warn-about-iteration-var-modifications-in-loo.patch --]
[-- Type: text/x-patch, Size: 10633 bytes --]

From d4fb1710bfa1d5b66979db1f0aea2d5c68ab2264 Mon Sep 17 00:00:00 2001
From: Frederik Harwath <frederik@harwath.name>
Date: Tue, 27 Feb 2024 21:07:00 +0000
Subject: [PATCH] OpenMP: warn about iteration var modifications in loop body

OpenMP loop iteration variables may not be changed by user code in the
loop body according to the OpenMP specification.  In general, the
compiler cannot enforce this, but nevertheless simple cases in which
the user modifies the iteration variable directly in the loop body
(in contrast to, e.g., modifications through a pointer) can be recognized. A
warning should be useful, for instance, to new users of OpenMP.

This commit implements a warning about forbidden iteration var modifications
during gimplification. It reuses the "loop_iter_var" vector in the
"gimplify_omp_ctx" which was previously only used for "doacross" handling to
identify the loop iteration variables during the gimplification of MODIFY_EXPRs
in omp_for bodies.

gcc/ChangeLog:

	* gimplify.cc (struct gimplify_omp_ctx): Add field "in_omp_for_body" to
	recognize the gimplification state during which the new warning should
	be emitted. Add field "is_doacross" to distinguish the original use of
	"loop_iter_var" from its new use.
	(new_omp_context): Initialize new gimplify_omp_ctx fields.
	(gimplify_modify_expr): Emit warning if iter var is modified.
	(gimplify_omp_for): Make initialization and filling of loop_iter_var
	vector unconditional and adjust new gimplify_omp_ctx fields before
	gimplifying the omp_for body.
	(gimplify_omp_ordered): Check for do_across field in addition to
	emptiness check on loop_iter_var vector since the vector is now always
	being filled.

gcc/testsuite/ChangeLog:

	* gcc.dg/vect/pr92347.c: Adjust.
	* gcc.target/aarch64/sve/pr96195.c: Adjust.
	* c-c++-common/gomp/iter-var-modification.c: New test.

Signed-off-by: Frederik Harwath  <frederik@harwath.name>
---
 gcc/gimplify.cc                               |  54 +++++++---
 .../c-c++-common/gomp/iter-var-modification.c | 100 ++++++++++++++++++
 gcc/testsuite/gcc.dg/vect/pr92347.c           |   2 +-
 .../gcc.target/aarch64/sve/pr96195.c          |   2 +-
 4 files changed, 140 insertions(+), 18 deletions(-)
 create mode 100644 gcc/testsuite/c-c++-common/gomp/iter-var-modification.c

diff --git a/gcc/gimplify.cc b/gcc/gimplify.cc
index 7f79b3cc7e6..a74ad987cf7 100644
--- a/gcc/gimplify.cc
+++ b/gcc/gimplify.cc
@@ -235,6 +235,8 @@ struct gimplify_omp_ctx
   bool order_concurrent;
   bool has_depend;
   bool in_for_exprs;
+  bool in_omp_for_body;
+  bool is_doacross;
   int defaultmap[5];
 };
 
@@ -456,6 +458,10 @@ new_omp_context (enum omp_region_type region_type)
   c->privatized_types = new hash_set<tree>;
   c->location = input_location;
   c->region_type = region_type;
+  c->loop_iter_var.create (0);
+  c->in_omp_for_body = false;
+  c->is_doacross = false;
+
   if ((region_type & ORT_TASK) == 0)
     c->default_kind = OMP_CLAUSE_DEFAULT_SHARED;
   else
@@ -6312,6 +6318,18 @@ gimplify_modify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
   gcc_assert (TREE_CODE (*expr_p) == MODIFY_EXPR
 	      || TREE_CODE (*expr_p) == INIT_EXPR);
 
+  if (gimplify_omp_ctxp && gimplify_omp_ctxp->in_omp_for_body)
+    {
+      size_t num_vars = gimplify_omp_ctxp->loop_iter_var.length () / 2;
+      for (size_t i = 0; i < num_vars; i++)
+	{
+	  if (*to_p == gimplify_omp_ctxp->loop_iter_var[2 * i + 1])
+	    warning_at (input_location, OPT_Wopenmp,
+			"forbidden modification of iteration variable %qE in "
+			"OpenMP loop", *to_p);
+	}
+    }
+
   /* Trying to simplify a clobber using normal logic doesn't work,
      so handle it here.  */
   if (TREE_CLOBBER_P (*from_p))
@@ -15334,6 +15352,8 @@ gimplify_omp_for (tree *expr_p, gimple_seq *pre_p)
 	      == TREE_VEC_LENGTH (OMP_FOR_COND (for_stmt)));
   gcc_assert (TREE_VEC_LENGTH (OMP_FOR_INIT (for_stmt))
 	      == TREE_VEC_LENGTH (OMP_FOR_INCR (for_stmt)));
+  int len = TREE_VEC_LENGTH (OMP_FOR_INIT (for_stmt));
+  gimplify_omp_ctxp->loop_iter_var.create (len * 2);
 
   tree c = omp_find_clause (OMP_FOR_CLAUSES (for_stmt), OMP_CLAUSE_ORDERED);
   bool is_doacross = false;
@@ -15342,8 +15362,6 @@ gimplify_omp_for (tree *expr_p, gimple_seq *pre_p)
     {
       OMP_CLAUSE_ORDERED_DOACROSS (c) = 1;
       is_doacross = true;
-      int len = TREE_VEC_LENGTH (OMP_FOR_INIT (for_stmt));
-      gimplify_omp_ctxp->loop_iter_var.create (len * 2);
       for (tree *pc = &OMP_FOR_CLAUSES (for_stmt); *pc; )
 	if (OMP_CLAUSE_CODE (*pc) == OMP_CLAUSE_LINEAR)
 	  {
@@ -15380,23 +15398,22 @@ gimplify_omp_for (tree *expr_p, gimple_seq *pre_p)
       gcc_assert (DECL_P (decl));
       gcc_assert (INTEGRAL_TYPE_P (TREE_TYPE (decl))
 		  || POINTER_TYPE_P (TREE_TYPE (decl)));
-      if (is_doacross)
+
+      if (TREE_CODE (for_stmt) == OMP_FOR && OMP_FOR_ORIG_DECLS (for_stmt))
 	{
-	  if (TREE_CODE (for_stmt) == OMP_FOR && OMP_FOR_ORIG_DECLS (for_stmt))
+	  tree orig_decl = TREE_VEC_ELT (OMP_FOR_ORIG_DECLS (for_stmt), i);
+	  if (TREE_CODE (orig_decl) == TREE_LIST)
 	    {
-	      tree orig_decl = TREE_VEC_ELT (OMP_FOR_ORIG_DECLS (for_stmt), i);
-	      if (TREE_CODE (orig_decl) == TREE_LIST)
-		{
-		  orig_decl = TREE_PURPOSE (orig_decl);
-		  if (!orig_decl)
-		    orig_decl = decl;
-		}
-	      gimplify_omp_ctxp->loop_iter_var.quick_push (orig_decl);
+	      orig_decl = TREE_PURPOSE (orig_decl);
+	      if (!orig_decl)
+		orig_decl = decl;
 	    }
-	  else
-	    gimplify_omp_ctxp->loop_iter_var.quick_push (decl);
-	  gimplify_omp_ctxp->loop_iter_var.quick_push (decl);
+	  gimplify_omp_ctxp->loop_iter_var.quick_push (orig_decl);
 	}
+      else
+	gimplify_omp_ctxp->loop_iter_var.quick_push (decl);
+      gimplify_omp_ctxp->loop_iter_var.quick_push (decl);
+
 
       if (for_stmt == orig_for_stmt)
 	{
@@ -15818,9 +15835,13 @@ gimplify_omp_for (tree *expr_p, gimple_seq *pre_p)
 	  TREE_SIDE_EFFECTS (OMP_FOR_BODY (orig_for_stmt)) = 1;
 	}
     }
+  gimplify_omp_ctxp->in_omp_for_body = true;
+  gimplify_omp_ctxp->is_doacross = is_doacross;
 
   gimple *g = gimplify_and_return_first (OMP_FOR_BODY (orig_for_stmt),
 					 &for_body);
+  gimplify_omp_ctxp->in_omp_for_body = false;
+  gimplify_omp_ctxp->is_doacross = false;
 
   if (TREE_CODE (orig_for_stmt) == OMP_TASKLOOP
       || (loop_p && orig_for_stmt == for_stmt))
@@ -17430,7 +17451,8 @@ gimplify_omp_ordered (tree expr, gimple_seq body)
     {
       for (c = OMP_ORDERED_CLAUSES (expr); c; c = OMP_CLAUSE_CHAIN (c))
 	if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_DOACROSS
-	    && gimplify_omp_ctxp->loop_iter_var.is_empty ())
+	    && (!gimplify_omp_ctxp->is_doacross
+		|| gimplify_omp_ctxp->loop_iter_var.is_empty ()))
 	  {
 	    error_at (OMP_CLAUSE_LOCATION (c),
 		      "%<ordered%> construct with %qs clause must be "
diff --git a/gcc/testsuite/c-c++-common/gomp/iter-var-modification.c b/gcc/testsuite/c-c++-common/gomp/iter-var-modification.c
new file mode 100644
index 00000000000..5662fce2a6f
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/gomp/iter-var-modification.c
@@ -0,0 +1,100 @@
+extern int a[1000];
+
+int main ()
+{
+#pragma omp for
+  for (int i = 0; i < 1000; i++)
+    {
+      if (i % 2  == 0)
+	i++; /* { dg-warning {forbidden modification of iteration variable .i. in OpenMP loop} } */
+    }
+
+  #pragma omp for
+  for (int i = 0; i < 1000; i++)
+    {
+      if (i % 2  == 0);
+      else
+	i++; /* { dg-warning {forbidden modification of iteration variable .i. in OpenMP loop} } */
+    }
+
+#pragma omp for
+  for (int i = 0; i < 1000; i++)
+    {
+	i = 0; /* { dg-warning {forbidden modification of iteration variable .i. in OpenMP loop} } */
+    }
+
+#pragma omp for
+  for (int i = 0; i != 1000; i++)
+    {
+	i = 0; /* { dg-warning {forbidden modification of iteration variable .i. in OpenMP loop} } */
+    }
+
+#pragma omp for
+  for (int i = 1000; i > 0; i--)
+    {
+	i = 0; /* { dg-warning {forbidden modification of iteration variable .i. in OpenMP loop} } */
+    }
+
+#pragma omp for
+  for (int *p = (int*)&a; p < a + 1000; p++)
+    {
+      p = (int*)&a; /* { dg-warning {forbidden modification of iteration variable .p. in OpenMP loop} } */
+    }
+
+#pragma omp for
+  for (int *p = (int*)&a; p < a + 1000; p++)
+    {
+	*p = 0;
+    }
+
+#pragma omp parallel for collapse(3)
+  for (int i = 0; i < 1000; i++)
+      for (int j = 0; j < 1000; j++)
+	for (int k = 0; k < 1000; k++)
+
+    {
+	j++; /* { dg-warning {forbidden modification of iteration variable .j. in OpenMP loop} } */
+    }
+
+#pragma omp target teams distribute parallel for collapse(3)
+  for (int i = 0; i < 1000; i++)
+      for (int j = 0; j < 1000; j++)
+	for (int k = 0; k < 1000; k++)
+
+    {
+	k++; /* { dg-warning {forbidden modification of iteration variable .k. in OpenMP loop} } */
+    }
+
+#pragma omp parallel for collapse(2)
+  for (int i = 0; i < 1000; i++)
+      for (int j = 0; j < 1000; j++)
+	for (int k = 0; k < 1000; k++)
+
+    {
+	j++; /* { dg-warning {forbidden modification of iteration variable .j. in OpenMP loop} } */
+    }
+
+#pragma omp target teams distribute parallel for collapse(2)
+  for (int i = 0; i < 1000; i++)
+      for (int j = 0; j < 1000; j++)
+	for (int k = 0; k < 1000; k++)
+
+    {
+	k++; /* No error since third loop is not included in collapsed loop-nest. */
+    }
+
+  /* Only modifications in the body should be reported.  Do not warn about
+     assignments to i,k in the pre-body. */
+  int i = 0;
+  int k = 0;
+#pragma omp target teams distribute parallel for collapse(3)
+  for (i = 1; i < 1000; i++)
+      for (int j = 0; j < 1000; j++)
+	for (k = 1; k < 1000; k++)
+
+    {
+	j++; /* { dg-warning {forbidden modification of iteration variable .j. in OpenMP loop} } */
+    }
+
+  return 0;
+}
diff --git a/gcc/testsuite/gcc.dg/vect/pr92347.c b/gcc/testsuite/gcc.dg/vect/pr92347.c
index f75b2596a26..2e1fc03b0c3 100644
--- a/gcc/testsuite/gcc.dg/vect/pr92347.c
+++ b/gcc/testsuite/gcc.dg/vect/pr92347.c
@@ -14,5 +14,5 @@ qh (int oh)
 {
 #pragma omp simd
   for (by = 0; by < oh; ++by)
-    by = zp (by);
+    by = zp (by); /* { dg-warning {forbidden modification of iteration variable .by. in OpenMP loop} } */
 }
diff --git a/gcc/testsuite/gcc.target/aarch64/sve/pr96195.c b/gcc/testsuite/gcc.target/aarch64/sve/pr96195.c
index d879efda5c8..3ef46e236e8 100644
--- a/gcc/testsuite/gcc.target/aarch64/sve/pr96195.c
+++ b/gcc/testsuite/gcc.target/aarch64/sve/pr96195.c
@@ -12,6 +12,6 @@ qh (int oh)
 {
 #pragma omp simd
   for (by = 0; by < oh; ++by)
-    by = zp (by);
+    by = zp (by); /* { dg-warning {forbidden modification of iteration variable .by. in OpenMP loop} } */
 }
 
-- 
2.34.1

Re: [PATCH] OpenMP: warn about iteration var modifications in loop body

[Jakub Jelinek]

On Wed, Mar 06, 2024 at 06:08:47PM +0100, Frederik Harwath wrote:
> Subject: [PATCH] OpenMP: warn about iteration var modifications in loop body

Note, the partially rewritten OpenMP loop transformations changes are now
in.
See below.

> --- a/gcc/gimplify.cc
> +++ b/gcc/gimplify.cc
> @@ -235,6 +235,8 @@ struct gimplify_omp_ctx
>    bool order_concurrent;
>    bool has_depend;
>    bool in_for_exprs;
> +  bool in_omp_for_body;
> +  bool is_doacross;
>    int defaultmap[5];
>  };
>  
> @@ -456,6 +458,10 @@ new_omp_context (enum omp_region_type region_type)
>    c->privatized_types = new hash_set<tree>;
>    c->location = input_location;
>    c->region_type = region_type;
> +  c->loop_iter_var.create (0);
> +  c->in_omp_for_body = false;
> +  c->is_doacross = false;

I'm not sure it is a good idea to reuse loop_iter_var for this.

>    if ((region_type & ORT_TASK) == 0)
>      c->default_kind = OMP_CLAUSE_DEFAULT_SHARED;
>    else
> @@ -6312,6 +6318,18 @@ gimplify_modify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
>    gcc_assert (TREE_CODE (*expr_p) == MODIFY_EXPR
>  	      || TREE_CODE (*expr_p) == INIT_EXPR);
>  
> +  if (gimplify_omp_ctxp && gimplify_omp_ctxp->in_omp_for_body)
> +    {
> +      size_t num_vars = gimplify_omp_ctxp->loop_iter_var.length () / 2;
> +      for (size_t i = 0; i < num_vars; i++)
> +	{
> +	  if (*to_p == gimplify_omp_ctxp->loop_iter_var[2 * i + 1])
> +	    warning_at (input_location, OPT_Wopenmp,
> +			"forbidden modification of iteration variable %qE in "
> +			"OpenMP loop", *to_p);

I think the forbidden word doesn't belong there, just modification of ...

Note, your patch seems to handle just one gimplify_omp_ctxp, not all.
If I do:
#pragma omp for
for (int i = 0; i < 32; ++i)
{
  ++i; // This is warned about
  #pragma omp parallel shared (i)
  #pragma omp master
  ++i; // This is not
  #pragma omp parallel private (i)
  ++i; // This should not
  #pragma omp target map(tofrom:i)
  ++i; // This is not
  #pragma omp target firstprivate (i)\
  ++i; // This should not
  #pragma omp simd
  for (i = 0; i < 32; ++i) // This is not
    ;
}
The question is if it isn't just too hard to figure out the data sharing
in nested constructs.  But to be useful, perhaps at least loop
transformation constructs which don't have any privatization on the
iterators (pending the resolution of the data sharing loop transformation
issue) should be handled.

> @@ -15380,23 +15398,22 @@ gimplify_omp_for (tree *expr_p, gimple_seq *pre_p)
>        gcc_assert (DECL_P (decl));
>        gcc_assert (INTEGRAL_TYPE_P (TREE_TYPE (decl))
>  		  || POINTER_TYPE_P (TREE_TYPE (decl)));
> -      if (is_doacross)
> +
> +      if (TREE_CODE (for_stmt) == OMP_FOR && OMP_FOR_ORIG_DECLS (for_stmt))

There is nothing specific about OMP_FOR for the orig decls, the reason
why the check is (probably) there is that simd construct has extra
restriction:
"The only random access iterator types that are allowed for the associated loops are pointer
types."
and so there is no point at looking at the orig decls for say for simd ordered(2)
doacross loops.
I was worried your patch wouldn't handle
void bar (int &);

void
foo ()
{
  int i;
  #pragma omp for
  for (i = 0; i < 32; ++i)
    bar (i);
}
where because the IV is addressable we actually choose to use an artificial
IV and assign i = i.0; at the start of the loop body, but apparently that
works right (though maybe it should go into the testsuite), supposedly we
emit it in gimplify_omp_for in GIMPLE before actually gimplifying the actual
OMP_FOR_BODY (but it is an assignment in there).

Anyway, what the patch certainly doesn't handle is the loop transformations.
The tile/unroll partial as done right now have the inter-tile emitted into
the OMP_FOR body, so both the initial assignment and the increment in there
would trigger the warning.  I guess similarly for reverse construct when
implemented.  Furthermore, the generated loops together with associated
ORIG_DECLs move to whatever outer construct loop needs them.

So, I think instead of doing it during gimplification of actual statements,
we should do it through a walk_tree on the bodies, done perhaps from inside
of omp_maybe_apply_loop_xforms or better right before that and mark through some
new flag loops whose bodies were walked for the diagnostics so that we don't
do that again.  Just have one hash map based on say DECL_UID into which we
mark all the loop iterators which should be warned about,
*walk_subtrees = 0; for OpenMP constructs which could privatize stuff
because it would be too difficult to handle but walk using a separate
walk_tree the loop transformation constructs and normally walk say
OMP_CRITICAL, OMP_MASKED and other constructs which never privatize stuff.
So, handle say
#pragma omp for
#pragma omp tile sizes (2, 2)
for (int i = 0; i < 32; ++i)
for (int j = 0; j < 32; ++j)
{
  ++i; // warn here; this is in the end generated loop of for, but before
       // lowering transformations actually on OMP_TILE
  ++j; // warn here; this is on OMP_TILE
  #pragma omp unroll partial (2)
  for (int k = 0; k < 32; ++k)
  {
    ++i; ++j; // warn on both here
    ++k; // and this
  }
}
etc.
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/gomp/iter-var-modification.c
> @@ -0,0 +1,100 @@
> +extern int a[1000];
> +
> +int main ()

Formatting.
int
main ()

> +{
> +#pragma omp for

Please have some formatting consistency.  Either #pragma omp is
indented the same as for below it, or it is 2 columns before that, but not
both.

> +  for (int i = 0; i < 1000; i++)
> +    {
> +      if (i % 2  == 0)

Why the 2 spaces in there?

> +	i++; /* { dg-warning {forbidden modification of iteration variable .i. in OpenMP loop} } */
> +    }
> +
> +  #pragma omp for
> +  for (int i = 0; i < 1000; i++)
> +    {
> +      if (i % 2  == 0);

The ; should be on a separate line.

> +      else
> +	i++; /* { dg-warning {forbidden modification of iteration variable .i. in OpenMP loop} } */
> +    }
> +
> +#pragma omp for
> +  for (int i = 0; i < 1000; i++)
> +    {
> +	i = 0; /* { dg-warning {forbidden modification of iteration variable .i. in OpenMP loop} } */
> +    }

No {}s around a single statement unless you test for that exact case
specially.  Perhaps have one case with it and others without.

> +#pragma omp for
> +  for (int i = 0; i != 1000; i++)
> +    {
> +	i = 0; /* { dg-warning {forbidden modification of iteration variable .i. in OpenMP loop} } */
> +    }
> +
> +#pragma omp for
> +  for (int i = 1000; i > 0; i--)
> +    {
> +	i = 0; /* { dg-warning {forbidden modification of iteration variable .i. in OpenMP loop} } */
> +    }
> +
> +#pragma omp for
> +  for (int *p = (int*)&a; p < a + 1000; p++)
> +    {
> +      p = (int*)&a; /* { dg-warning {forbidden modification of iteration variable .p. in OpenMP loop} } */
> +    }
> +
> +#pragma omp for
> +  for (int *p = (int*)&a; p < a + 1000; p++)
> +    {
> +	*p = 0;
> +    }
> +
> +#pragma omp parallel for collapse(3)
> +  for (int i = 0; i < 1000; i++)
> +      for (int j = 0; j < 1000; j++)
> +	for (int k = 0; k < 1000; k++)
> +

No blank line.  And the formatting here is just weird, 2 spaces, 6 spaces,
tab?  Should be 2, 4, 6.

> +    {

And 4 spaces here?  Should be tab.
Etc.

> --- a/gcc/testsuite/gcc.dg/vect/pr92347.c
> +++ b/gcc/testsuite/gcc.dg/vect/pr92347.c
> @@ -14,5 +14,5 @@ qh (int oh)
>  {
>  #pragma omp simd
>    for (by = 0; by < oh; ++by)
> -    by = zp (by);
> +    by = zp (by); /* { dg-warning {forbidden modification of iteration variable .by. in OpenMP loop} } */

I think the testcases should be just fixed not to do that.
So, if it needs to store something, store into some array, arr[by] = zp (by);
or so.

> --- a/gcc/testsuite/gcc.target/aarch64/sve/pr96195.c
> +++ b/gcc/testsuite/gcc.target/aarch64/sve/pr96195.c
> @@ -12,6 +12,6 @@ qh (int oh)
>  {
>  #pragma omp simd
>    for (by = 0; by < oh; ++by)
> -    by = zp (by);
> +    by = zp (by); /* { dg-warning {forbidden modification of iteration variable .by. in OpenMP loop} } */
>  }

Likewise.

	Jakub