[Bug fortran/107424] [13 Regression] ICE in gfc_trans_omp_do, at fortran/trans-openmp.cc:5397 - and wrong code - with non-rectangular loops

["burnus at gcc dot gnu.org" <gcc-bugzilla@gcc.gnu.org>]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107424

Tobias Burnus <burnus at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |accepts-invalid

--- Comment #7 from Tobias Burnus <burnus at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #6)
> I think there are multiple issues.  For the do i = 1, 9, 1 case, the bug is
> that we:
>       to = gfc_evaluate_now (se.expr, pblock);

The patch simply skips this if se.expr is already a DECL_P (as it is then IMHO
pointless)  or if there is a non-rectangular loop nest. — In the latter case,
it additionally replaces
 'inner = outer + a1' by  'inner = (count.outer + lb.outer) + a1'

Thus, semantically, that should be fine but breaks with:
...

> If the inner loop of non-rectangular loop nest (well, in particular whenever
> from or to
> uses some outer loop iterator and is one of the allowed non-rectangular
> forms):
...
> var-outer + a2
...
> var-outer * a1 + a2
> then using the non-simple expansion just can't work, the middle-end expects
> the init and cond expressions to be literally something from the above list,

In the testcase I have - which does not use 'count' - the original dump for
Fortran is:
      for (k = j + -41; k <= p; k = k + 1)
but for C it is
      for (k = j * 1 + -41; k <= p; k++ )

Both are valid according to the list, but possibly the gimplifier does not
handle
  'outer + a2' but only 'outer * a1 + a2'
which would explain the issue I see. From the code, it also looks as if a very
specific order is required – and a TREE_VEC and not a normal expression:

      if (OMP_FOR_NON_RECTANGULAR (for_stmt) && var != decl)
        for (int j = i + 1; j < TREE_VEC_LENGTH (OMP_FOR_INIT (for_stmt)); j++)
          {
            t = TREE_VEC_ELT (OMP_FOR_INIT (for_stmt), j);
            gcc_assert (TREE_CODE (t) == MODIFY_EXPR);
            if (TREE_CODE (TREE_OPERAND (t, 1)) == TREE_VEC
                && TREE_VEC_ELT (TREE_OPERAND (t, 1), 0) == decl)
              TREE_VEC_ELT (TREE_OPERAND (t, 1), 0) = var;

 * * *

With regards to 'count', I think it would still work - but the replacement
    ll * 3 + a1
cannot be: (count.0 + lb) * 3 + a1
but has to be:  count.0 * 3 + (lb*3 + a1)

Given that already quite some specific format is required (see above), it seems
as if
just continuing to use 'count' would work.
It also would permit to add some additional checks, ensuring that the format of
the
loop is correct.

It also seems to be simpler than:

> Now, the reason I've added the above expansion is because the middle-end representation
> was chosen to be of the C/C++ loops and I wasn't sure if the Fortran DO loop semantics
> can be easily mapped to it.  For the non-rectangular loops, I'm afraid we have to,
> or have some flag on each of the OMP_FOR/GOMP_FOR loops to request Fortran semantics.


Note regarding:
> if that is turned into something divided by step, we've lost.

I note that for Fortran, the OpenMP spec (I looked at 5.2) has:

   lb, ub   One of the following:
        ....
   a1, a2, incr   Integer expressions that are loop invariant with respect
                  to the outermost loop of the10 loop nest.

Thus, as soon as a DO loop has an increment expression (even if it evaluates to
'1'),
neither a1 nor a2 nor incr  can depend on any outer loop variable.

Thus, it seems to be doable in the Fortran FE itself.

Regarding the latter, gfortran accepts:
      do k = i - 41, p, p *4
which I think is wrong ('i - 41' does not fulfill this requirement)'. Likewise
wrong
is
      do k = i - 41, p, 1
but I am not sure whether the ',1' vs. absent is trivially detectable or not.