Updated patch. Changes: the canonic(al) and the if/else(if) cosmetic
formatting changes.

Testcases: Additionally, I checked for the value of a zero-loop trip in
libgomp.fortran/non-rectangular-loop-1.f90 and added lastprivate to all
of .../non-rectangular-loop-{3,4}.f90 (unless the loop-iteration
variable is a pointer).

At least those three files pass with and without -fopenmp, implying that
there the lastprivate check is correct. ('1a' and '5' also use
lastprivate, but those fail with a 'sorry' before a value test can be
done.) In '1' there are also several tests with 'parallel do', but only
SIMD has been tested more extensively (in general and for lastprivate).

I think the test coverage should be sufficient. Any further test idea?
Otherwise, I would commit it now.

Tobias

PS: Next planned by me: revising the 'omp loop' patch and a longer
pending 'map(alloc:' patch. And at some point supporting at least some
non-±1 increments with non-rect loop nests.

On 31.01.23 12:37, Jakub Jelinek wrote:
> On Wed, Jan 25, 2023 at 03:47:18PM +0100, Tobias Burnus wrote:
>> updated patch included, i.e. avoiding 'count' for 'j' when a 'j.0' would
>> do (i.e. only local var without the different step calculation). I also
>> now reject if there is a non-unit step on the loop using an outer var.
>>
>> Eventually still to be done: replace the 'sorry' by working code, i.e.
>> implement the suggestions to handle some/all non-unit iteration steps as
>> proposed in this thread.
>>
>> On 20.01.23 18:39, Jakub Jelinek wrote:
>>> I think instead of non-unity etc. it is better to talk about constant
>>> step 1 or -1.
>> I concur.
>>
>>
>>> The actual problem with non-simple loops for non-rectangular loops is
>>> both in case it is an inner loop which uses some outer loop's iterator,
>>> or if it is outer loop whose iterator is used, both of those cases
>>> will not be handled properly.
>> I have now added a check for the other case as well.
>>
>> Just to confirm, the following is fine, isn't it?
>>
>> !$omp simd collapse(4)
>> do i = 1, 10, 2
>>    do outer_var = 1, 10  ! step = + 1
>>      do j = 1, 10, 2
>>        do inner_var = 1, outer_var  ! step = 1
>>
>> i.e. both the inner_var and outer_var have 'step = 1',
>> even if other loops in the 'collapse' have step != 1.
>> I think it should be fine.
> Yes, the loops which don't define outer_var for other loops nor
> use outer_var from other loops can be in any form, we can compute
> their number of iterations before the whole loop nest for them,
> so for the non-rectangular iterations computations we can ignore
> those except for multiplication by the pre-computed count.
>
>> OpenMP/Fortran: Partially fix non-rect loop nests [PR107424]
>>
>> This patch ensures that loop bounds depending on outer loop vars use the
>> proper TREE_VEC format. It additionally gives a sorry if such an outer
>> var has a non-one/non-minus-one increment as currently a count variable
>> is used in this case (see PR).
>>
>> Finally, it avoids 'count' and just uses a local loop variable if the
>> step increment is +/-1.
>>
>>      PR fortran/107424
>>
>> gcc/fortran/ChangeLog:
>>
>>      * trans-openmp.cc (struct dovar_init_d): Add 'sym' and
>>      'non_unit_incr' members.
>>      (gfc_nonrect_loop_expr): New.
>>      (gfc_trans_omp_do): Call it; use normal loop bounds
>>      for unit stride - and only create local loop var.
>>
>> libgomp/ChangeLog:
>>
>>      * testsuite/libgomp.fortran/non-rectangular-loop-1.f90: New test.
>>      * testsuite/libgomp.fortran/non-rectangular-loop-1a.f90: New test.
>>      * testsuite/libgomp.fortran/non-rectangular-loop-2.f90: New test.
>>      * testsuite/libgomp.fortran/non-rectangular-loop-3.f90: New test.
>>      * testsuite/libgomp.fortran/non-rectangular-loop-4.f90: New test.
>>      * testsuite/libgomp.fortran/non-rectangular-loop-5.f90: New test.
>>
>> gcc/testsuite/ChangeLog:
>>
>>      * gfortran.dg/goacc/privatization-1-compute-loop.f90: Update dg-note.
>>      * gfortran.dg/goacc/privatization-1-routine_gang-loop.f90: Likewise.
>>
>> +static bool
>> +gfc_nonrect_loop_expr (stmtblock_t *pblock, gfc_se *sep, int loop_n,
>> +                   gfc_code *code, gfc_expr *expr, vec<dovar_init> *inits,
>> +                   int simple, gfc_expr *curr_loop_var)
>> +{
>> +  int i;
>> +  for (i = 0; i < loop_n; i++)
>> +    {
>> +      gcc_assert (code->ext.iterator->var->expr_type == EXPR_VARIABLE);
>> +      if (gfc_find_sym_in_expr (code->ext.iterator->var->symtree->n.sym, expr))
>> +    break;
>> +      code = code->block->next;
>> +    }
>> +  if (i >= loop_n)
>> +    return false;
>> +
>> +  /* Canonic format: TREE_VEC with [var, multiplier, offset].  */
> I think we use everywhere Canonical rather than Canonic
>
>> +  gfc_symbol *var = code->ext.iterator->var->symtree->n.sym;
>> +
>> +  tree tree_var = NULL_TREE;
>> +  tree a1 = integer_one_node;
>> +  tree a2 = integer_zero_node;
>> +
>> +  if (!simple)
>> +    {
>> +      /* FIXME: Handle non-unit iter steps, cf. PR fortran/107424.  */
>> +      sorry_at (gfc_get_location (&curr_loop_var->where),
>> +            "non-rectangular loop nest with step other than constant 1 "
>> +            "or -1 for %qs", curr_loop_var->symtree->n.sym->name);
>> +      return false;
>> +    }
>> +
>> +  dovar_init *di;
>> +  unsigned ix;
>> +  FOR_EACH_VEC_ELT (*inits, ix, di)
>> +    if (di->sym == var && !di->non_unit_iter)
>> +      {
>> +    tree_var = di->init;
>> +    gcc_assert (DECL_P (tree_var));
>> +    break;
>> +      }
>> +    else if (di->sym == var)
>> +      {
>> +    /* FIXME: Handle non-unit iter steps, cf. PR fortran/107424.  */
>> +    sorry_at (gfc_get_location (&code->loc),
>> +            "non-rectangular loop nest with step other than constant 1 "
>> +            "or -1 for %qs", var->name);
>> +    inform (gfc_get_location (&expr->where), "Used here");
>> +    return false;
>> +      }
> I think it would be better formatted as
>      if (di->sym == var)
>        {
>       if (!di->non_unit_iter)
>         {
>           ...
>         }
>       else
>         {
>           ...
>         }
>        }
>
>> +      if (simple && !DECL_P (dovar))
>> +    {
>> +      const char *name = code->ext.iterator->var->symtree->n.sym->name;
>> +      local_dovar = gfc_create_var (type, name);
>> +      dovar_init e = {code->ext.iterator->var->symtree->n.sym,
>> +                      dovar, local_dovar, false};
>> +      inits.safe_push (e);
>> +    }
> For the separate local_dovar case, I'd be worried if we handle lastprivate
> right.  From quick skimming I see some lastprivate clauses in some of
> the tests, so if they verify the right value has been computed (say the
> same as one would get with -fno-openmp), then fine.
Tobias
-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955