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