[Bug fortran/102043] [9/10/11/12 Regression] Wrong array types used for negative stride accesses, gfortran.dg/vector_subscript_1.f90 FAILs

[cvs-commit at gcc dot gnu.org via Gcc-bugs]

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

--- Comment #47 from CVS Commits  ---
The master branch has been updated by Mikael Morin :

https://gcc.gnu.org/g:7964ab6c364c410c34efe7ca2eba797d36525349

commit r12-8230-g7964ab6c364c410c34efe7ca2eba797d36525349
Author: Mikael Morin 
Date:   Fri Apr 22 22:52:50 2022 +0200

fortran: Use pointer arithmetic to index arrays [PR102043]

The code generated for array references used to be ARRAY_REF trees as
could be expected.  However, the middle-end may conclude from those
trees that the indexes used are non-negative (more precisely not below
the lower bound), which is a wrong assumption in the case of "reversed-
order" arrays.

The problematic arrays are those with a descriptor and having a negative
stride for at least one dimension.  The descriptor data points to the
first element in array order (which is not the first in memory order in
that case), and the negative stride(s) makes walking the array backwards
(towards lower memory addresses), and we can access elements with
negative index wrt data pointer.

With this change, pointer arithmetic is generated by default for array
references, unless we are in a case where negative indexes canât happen
(array descriptorâs dim element, substrings, explicit shape,
allocatable, or assumed shape contiguous).  A new flag is added to
choose between array indexing and pointer arithmetic, and itâs set
if the context can tell array indexing is safe (descriptor dim
element, substring, temporary array), or a new method is called
to decide on whether the flag should be set for one given array
expression.

PR fortran/102043

gcc/fortran/ChangeLog:

* trans.h (gfc_build_array_ref): Add non_negative_offset
argument.
* trans.cc (gfc_build_array_ref): Ditto. Use pointer arithmetic
if non_negative_offset is false.
* trans-expr.cc (gfc_conv_substring): Set flag in the call to
gfc_build_array_ref.
* trans-array.cc (gfc_get_cfi_dim_item,
gfc_conv_descriptor_dimension): Same.
(build_array_ref): Decide on whether to set the flag and update
the call.
(gfc_conv_scalarized_array_ref): Same.  New argument tmp_array.
(gfc_conv_tmp_array_ref): Update call to
gfc_conv_scalarized_ref.
(non_negative_strides_array_p): New function.

gcc/testsuite/ChangeLog:

* gfortran.dg/array_reference_3.f90: New.
* gfortran.dg/negative_stride_1.f90: New.
* gfortran.dg/vector_subscript_8.f90: New.
* gfortran.dg/vector_subscript_9.f90: New.
* gfortran.dg/c_loc_test_22.f90: Update dump patterns.
* gfortran.dg/finalize_10.f90: Same.

Co-Authored-By: Richard Biener 

[Bug fortran/102043] [9/10/11/12 Regression] Wrong array types used for negative stride accesses, gfortran.dg/vector_subscript_1.f90 FAILs

[cvs-commit at gcc dot gnu.org via Gcc-bugs]

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

--- Comment #46 from CVS Commits  ---
The master branch has been updated by Mikael Morin :

https://gcc.gnu.org/g:761dda57482295f9c41fcf87e5defa2ac1959f03

commit r12-8229-g761dda57482295f9c41fcf87e5defa2ac1959f03
Author: Mikael Morin 
Date:   Fri Apr 22 22:52:38 2022 +0200

fortran: Generate an array temporary reference [PR102043]

This avoids regressing on char_cast_1.f90 and char_cast_2.f90 later in
the patch series when the code generation for array references is
changed to use pointer arithmetic.

The regressing testcases match part of an array reference in the
generated tree dump and itâs not clear how the pattern should be
rewritten to match the equivalent with pointer arithmetic.

This change uses a method specific to array temporaries to generate
array-references, so that these array references are flagged as safe
for array indexing and will not be updated to use pointer arithmetic.

PR fortran/102043

gcc/fortran/ChangeLog:
* trans-array.cc (gfc_conv_expr_descriptor): Use
gfc_conv_tmp_array_ref.

[Bug fortran/102043] [9/10/11/12 Regression] Wrong array types used for negative stride accesses, gfortran.dg/vector_subscript_1.f90 FAILs

[cvs-commit at gcc dot gnu.org via Gcc-bugs]

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

--- Comment #45 from CVS Commits  ---
The master branch has been updated by Mikael Morin :

https://gcc.gnu.org/g:e72fbb6915c1dd1a52ecef55e10329e353cc3072

commit r12-8228-ge72fbb6915c1dd1a52ecef55e10329e353cc3072
Author: Mikael Morin 
Date:   Fri Apr 22 22:52:26 2022 +0200

fortran: Update index extraction code. [PR102043]

This avoids a regression on hollerith4.f90 and hollerith6.f90 later in
the patch series when code generation for array references is changed
to use pointer arithmetic.

The problem comes from the extraction of the array index from an
ARRAY_REF tree, which doesnât work if the tree is not an ARRAY_REF
any more.

This updates the code generated for remaining size evaluation to work
with a source tree that uses either array indexing or pointer
arithmetic.

PR fortran/102043

gcc/fortran/ChangeLog:

* trans-io.cc: Add handling for the case where the array
is referenced using pointer arithmetic.

[Bug fortran/102043] [9/10/11/12 Regression] Wrong array types used for negative stride accesses, gfortran.dg/vector_subscript_1.f90 FAILs

[cvs-commit at gcc dot gnu.org via Gcc-bugs]

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

--- Comment #44 from CVS Commits  ---
The master branch has been updated by Mikael Morin :

https://gcc.gnu.org/g:89ca0fffa48b799b228beee48a16e26e24d8e199

commit r12-8227-g89ca0fffa48b799b228beee48a16e26e24d8e199
Author: Mikael Morin 
Date:   Fri Apr 22 22:52:12 2022 +0200

fortran: Pre-evaluate string pointers. [PR102043]

This avoids a regression on deferred_character_23.f90 later in the
patch series when array references are rewritten to use pointer
arithmetic.

The problem is a SAVE_EXPR tree as TYPE_SIZE_UNIT of one array element
type, which is used by the pointer arithmetic expressions.  As these
expressions appear in both branches of an if-then-else block, the tree
is lowered to a variable in one of the branches but itâs used in both
branches, which is invalid middle-end code.

This change pre-evaluates the array references or pointer arithmetics
to variables before the if-then-else block, so that the SAVE_EXPR are
expanded to variables in the parent scope of the if-then-else block,
and expressions referencing the variables remain valid in both
branches.

PR fortran/102043

gcc/fortran/ChangeLog:
* trans-expr.cc: Pre-evaluate src and dest to variables
before using them.

gcc/testsuite/ChangeLog:
* gfortran.dg/dependency_49.f90: Update variable occurence
count.

[Bug fortran/102043] [9/10/11/12 Regression] Wrong array types used for negative stride accesses, gfortran.dg/vector_subscript_1.f90 FAILs

[mikael at gcc dot gnu.org via Gcc-bugs]

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

Mikael Morin  changed:

   What|Removed |Added

   Assignee|unassigned at gcc dot gnu.org  |mikael at gcc dot 
gnu.org
 Status|NEW |ASSIGNED

--- Comment #43 from Mikael Morin  ---
I’m working on it.

[Bug fortran/102043] [9/10/11/12 Regression] Wrong array types used for negative stride accesses, gfortran.dg/vector_subscript_1.f90 FAILs

[rguenth at gcc dot gnu.org via Gcc-bugs]

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

--- Comment #42 from Richard Biener  ---
Created attachment 52717
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=52717=edit
hack to rewrite all ARRA_REFs

This shows a simple hack emitting *( + offset) from gfc_build_array_ref. 
That covers too many cases (as said, covering cases from array descriptor
accesses should be enough).  Appended is also testresults which show besides a
large number of original tree dump scanning fails a few execute FAILs and a few
ICEs where the gfortran frontend doesn't expect anything but an ARRAY_REF to be
fed into select places (some already deal with an INDIRECT_REF as I found out).
One such place is gfc_convert_array_to_string.

As said, all this is probably backwards as to what the OpenACC folks desire.

[Bug fortran/102043] [9/10/11/12 Regression] Wrong array types used for negative stride accesses, gfortran.dg/vector_subscript_1.f90 FAILs

[rguenth at gcc dot gnu.org via Gcc-bugs]

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

--- Comment #41 from Richard Biener  ---
-- random ideas dumping below --

so reading about ISO_Fortran_binding.h it seems to be that a base of the object
isn't readily available and making it available would require quite some
computation.

staying with the current scheme of simply piggy-backing on C [] operator
semantics which is, when facing a pointer that is being indexed, equivalent
to pointer arithmetic which _can_ advance to before the indexed pointer,
looks reasonable.

that rules out using ARRAY_REF for all accesses through a descriptor
(unless all strides are positive - not sure if it's worth special casing that
though).

it might be helpful to have a tree code providing CFI_address in an expanded
form with explicitely specified index, [low-bound,] stride per indexed
dimension and with the guarantee of CFI that the dimensions are independent
(non-overlapping elements).  I'd call it ELEMENT_SELECT_EXPR here, it would
not be a tcc_reference since it only computes an address that would need
to be dereferenced with a MEM_REF.  Since it would be variable-length it
doesn't nicely map to GIMPLE.  Lowering during gimplification would be
possible.  Note we already have a vehicle that should be usable for
the 1-dimensional case, namely TARGET_MEM_REF which allows variable
pointer offsetting with a scaled index.  Of course it would be somewhat
abusing this and dependence analysis doesn't play nicely with it either
AFAIK.  Going for selected ARRAY_REF -> POINTER_PLUS_EXPR + MEM_REF
lowering sounds like the way of least resistance, but then that's probably
backwards of what the OpenACC folks want to do.

[Bug fortran/102043] [9/10/11/12 Regression] Wrong array types used for negative stride accesses, gfortran.dg/vector_subscript_1.f90 FAILs

[rguenther at suse dot de via Gcc-bugs]

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

--- Comment #40 from rguenther at suse dot de  ---
On Wed, 30 Mar 2022, tkoenig at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102043
> 
> --- Comment #39 from Thomas Koenig  ---
> (In reply to Richard Biener from comment #37)
> 
> > The issue itself is long latent so we probably have to live with GCC 12
> > exposing slightly more cases of it in the real world (I have yet to see
> > one there).
> 
> Silent introduction of wrong code is always a bad idea.
> 
> Should we try to generate an ICE for the test case, so people are at
> least warned?

An ICE for every (possibly!) negative stride array in Fortran?!

[Bug fortran/102043] [9/10/11/12 Regression] Wrong array types used for negative stride accesses, gfortran.dg/vector_subscript_1.f90 FAILs

[tkoenig at gcc dot gnu.org via Gcc-bugs]

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

--- Comment #39 from Thomas Koenig  ---
(In reply to Richard Biener from comment #37)

> The issue itself is long latent so we probably have to live with GCC 12
> exposing slightly more cases of it in the real world (I have yet to see
> one there).

Silent introduction of wrong code is always a bad idea.

Should we try to generate an ICE for the test case, so people are at
least warned?

[Bug fortran/102043] [9/10/11/12 Regression] Wrong array types used for negative stride accesses, gfortran.dg/vector_subscript_1.f90 FAILs

[burnus at gcc dot gnu.org via Gcc-bugs]

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

--- Comment #38 from Tobias Burnus  ---
(In reply to Thomas Koenig from comment #15)
> One possibility would be to extend the patch Sandra posted at
> https://gcc.gnu.org/pipermail/fortran/2021-January/055563.html
> to scalarization.

As mentioned by Thomas, a re-based patch is
https://gcc.gnu.org/pipermail/gcc-patches/2021-November/584716.html

(I did not quickly see whether Mikael's patch c, attachment 51974) is
completely orthogonal, goes in the same direction or a different one.)


Mikael Morin wrote in (comment #35)
> I have tried to fix this PR using pointer arithmetic too.
> But there are so many places in the frontend where we expect to have an
> array type when dereferencing a descriptor pointer

Do you see a special issue here or not?

In that area, I realized when working on my OpenMP deep-mapping patch that
there were issues related to coarrays and CLASS, in particular:
* 'select type' always adds 'attr.pointer' which both permits wrong code (e.g.
deallocate, pointer assignment), but also sets the GFC_ARRAY_POINTER - but
fixing it then caused issues with coarrays.
(GFC_TYPE_ARRAY_AKIND also does not distinguish allocatable/pointer for assumed
rank, but that should be only/mostly a problem for my OpenMP patch.)


I have a continuously growing to-do list, but still: is there something I can
do here?

[Bug fortran/102043] [9/10/11/12 Regression] Wrong array types used for negative stride accesses, gfortran.dg/vector_subscript_1.f90 FAILs

[rguenth at gcc dot gnu.org via Gcc-bugs]

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

--- Comment #37 from Richard Biener  ---
(In reply to rguent...@suse.de from comment #36)
> On Sun, 27 Mar 2022, mikael at gcc dot gnu.org wrote:
> 
> > For gcc-12, is there a way to add a middle-end workaround using annotations 
> > on
> > descriptor types (a lang flag or something) ?
> 
> I will think of what options we have to work around this in the
> middle-end (maybe with help of the frontend during gimplification).

So the difficult issue is that the intended effect of the ARRAY_REF is to
apply a negative offset to its base.  That's something fundamentally not
supported for any handled component reference, especially when it is a
non-constant negative offset.  An access is currently constrained to
[offset, offset + max_size] where max_size can be unknown (we use a special
value of -1 for that).  In particular 'offset' cannot be unknown, but when
we split the ultimate base object we do support a constant negative offset
from the real base object.  So we consider a get_base_address() base
plus [0, 0 + unknown] to be a valid conservative answer - but for the
fortran ARRAY_REF indexing it would not be!

Now - actual miscompilations of course require more specific setups, the
ones cited here take advantage of knowing the size of the underlying object
and together with the offset >= 0 constraint constraining the variable index
to exactly the last element of the array.  We already have
-funconstrained-commons to catch a similar case (but with trailing arrays
here).

On the middle-end side it's hard to tell whether the actual array
reference is from a "safe" one (not via an array descriptor and not with
negative stride), so truly fixing it there with lowering the ARRAY_REF
to pointer arithmetic would be a mass-rewrite of even safe accesses.

Another effect besides the wrong constant propagation would be
disambiguation of accesses.  Like if we had

  integer, dimension (10) :: a
  do i = 1, 10
reshape(a, (/10:1:-1/))(i) = a(i)
  end do

(sorry for my likely invalid fortran, but you get the idea), then
the middle-end would disambiguate all a(i) besides a(10) against the
store.

To sum up, I don't see a good way to workaround this in the middle-end
(without a really big hammer that would do more harm than good).  I also
do not see a way to teach the middle-end negative offsetting ARRAY_REFs,
even if you'd call them differently.

The issue itself is long latent so we probably have to live with GCC 12
exposing slightly more cases of it in the real world (I have yet to see
one there).

[Bug fortran/102043] [9/10/11/12 Regression] Wrong array types used for negative stride accesses, gfortran.dg/vector_subscript_1.f90 FAILs

[rguenther at suse dot de via Gcc-bugs]

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

--- Comment #36 from rguenther at suse dot de  ---
On Sun, 27 Mar 2022, mikael at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102043
> 
> --- Comment #35 from Mikael Morin  ---
> A little status update.
> 
> I have pushed the latest patch attached to this PR a little further, but not
> far enough to reduce the number of testsuite regressions to 0.
> 
> I plan to submit it for gcc-13, but it won’t make it for gcc-12.
> 
> (In reply to Richard Biener from comment #33)
> > With
> > a single dimension there's not much value in using ARRAY_REF over
> > pointer arithmetic and dereference.
> 
> I have tried to fix this PR using pointer arithmetic too.
> But there are so many places in the frontend where we expect to have an array
> type when dereferencing a descriptor pointer, that it’s not as simple as it
> seems and I finally convinced myself that the patch attached to this PR was 
> the
> best way to go.

OK.

> For gcc-12, is there a way to add a middle-end workaround using annotations on
> descriptor types (a lang flag or something) ?

I will think of what options we have to work around this in the
middle-end (maybe with help of the frontend during gimplification).

[Bug fortran/102043] [9/10/11/12 Regression] Wrong array types used for negative stride accesses, gfortran.dg/vector_subscript_1.f90 FAILs

[mikael at gcc dot gnu.org via Gcc-bugs]

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

--- Comment #35 from Mikael Morin  ---
A little status update.

I have pushed the latest patch attached to this PR a little further, but not
far enough to reduce the number of testsuite regressions to 0.

I plan to submit it for gcc-13, but it won’t make it for gcc-12.

(In reply to Richard Biener from comment #33)
> With
> a single dimension there's not much value in using ARRAY_REF over
> pointer arithmetic and dereference.

I have tried to fix this PR using pointer arithmetic too.
But there are so many places in the frontend where we expect to have an array
type when dereferencing a descriptor pointer, that it’s not as simple as it
seems and I finally convinced myself that the patch attached to this PR was the
best way to go.

For gcc-12, is there a way to add a middle-end workaround using annotations on
descriptor types (a lang flag or something) ?

[Bug fortran/102043] [9/10/11/12 Regression] Wrong array types used for negative stride accesses, gfortran.dg/vector_subscript_1.f90 FAILs

[tschwinge at gcc dot gnu.org via Gcc-bugs]

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

Thomas Schwinge  changed:

   What|Removed |Added

 CC||burnus at gcc dot gnu.org,
   ||frederik at gcc dot gnu.org,
   ||sandra at gcc dot gnu.org,
   ||tschwinge at gcc dot gnu.org

--- Comment #34 from Thomas Schwinge  ---
I'm totally lacking all context here ;-) -- but did see "OpenACC" mentioned,
so:

(In reply to Richard Biener from comment #33)
> I'll note the OpenACC folks were working on making gfortran preserve
> multi-dimensional array accesses

See Frederik's submission "Fortran: Delinearize array accesses",
,
primarily developed by Sandra Loosemore, with help from Tobias Burnus (from
what I remember).  Intended for discussion for GCC 13.

> not sure how they address this issue.  With
> a single dimension there's not much value in using ARRAY_REF over
> pointer arithmetic and dereference.

[Bug fortran/102043] [9/10/11/12 Regression] Wrong array types used for negative stride accesses, gfortran.dg/vector_subscript_1.f90 FAILs

[rguenth at gcc dot gnu.org via Gcc-bugs]

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

Richard Biener  changed:

   What|Removed |Added

   Target Milestone|--- |9.5
Summary|Wrong array types used for  |[9/10/11/12 Regression]
   |negative stride accesses|Wrong array types used for
   ||negative stride accesses,
   ||gfortran.dg/vector_subscrip
   ||t_1.f90  FAILs

--- Comment #33 from Richard Biener  ---
Btw, for GCC 12 this issue is now visible through IPA modref and results in

FAIL: gfortran.dg/vector_subscript_1.f90   -O1  execution test
FAIL: gfortran.dg/vector_subscript_1.f90   -O2  execution test
FAIL: gfortran.dg/vector_subscript_1.f90   -O3 -fomit-frame-pointer
-funroll-loops -fpeel-loops -ftracer -finline-functions  execution test
FAIL: gfortran.dg/vector_subscript_1.f90   -O3 -g  execution test
FAIL: gfortran.dg/vector_subscript_1.f90   -Os  execution test

on at least x86_64-unknown-linux-gnu.

The testcase in comment#1 still shows the issue is older.  Since gfortran 4.3
works this is still a regression (I guess it became appearant with the alias
oracle introduction).

I'll note the OpenACC folks were working on making gfortran preserve
multi-dimensional array accesses, not sure how they address this issue.  With
a single dimension there's not much value in using ARRAY_REF over
pointer arithmetic and dereference.