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

[Jakub Jelinek <jakub@redhat.com>]

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