Bug 45586 - [4.8 Regression] ICE non-trivial conversion at assignment
Summary: [4.8 Regression] ICE non-trivial conversion at assignment
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: fortran (show other bugs)
Version: 4.8.0
: P4 normal
Target Milestone: 4.8.3
Assignee: Not yet assigned to anyone
URL: http://gcc.gnu.org/ml/fortran/2012-08...
Keywords: ice-checking, ice-on-valid-code, lto, wrong-code
: 52516 53318 (view as bug list)
Depends on: 45777
Blocks: 52651
  Show dependency treegraph
 
Reported: 2010-09-07 19:23 UTC by Joost VandeVondele
Modified: 2014-02-16 07:00 UTC (History)
17 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2013-08-29 00:00:00


Attachments
patch (1.07 KB, patch)
2010-10-21 15:28 UTC, Richard Biener
Details | Diff
possible patch (1.33 KB, patch)
2011-01-20 16:36 UTC, Michael Matz
Details | Diff
rough patch (5.32 KB, patch)
2012-08-01 12:22 UTC, Mikael Morin
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joost VandeVondele 2010-09-07 19:23:18 UTC
I'm trying to do an lto build of CP2K, but this fails with:

/data03/vondele/clean/cp2k/makefiles/../src/qs_linres_current.F: In function ‘calculate_jrho_resp’:
/data03/vondele/clean/cp2k/makefiles/../src/qs_linres_current.F:614:0: error: non-trivial conversion at assignment
struct array3_real(kind=8)
struct array3_real(kind=8)
# .MEM_3681 = VDEF <.MEM_3286>
my_rho = D.13693_938->r;

/data03/vondele/clean/cp2k/makefiles/../src/qs_linres_current.F:614:0: error: non-trivial conversion at assignment
struct array3_real(kind=8)
struct array3_real(kind=8)
# .MEM_3682 = VDEF <.MEM_3681>
my_current = D.13701_946->r;

/data03/vondele/clean/cp2k/makefiles/../src/qs_linres_current.F:614:0: error: non-trivial conversion at assignment
struct array3_real(kind=8)
struct array3_real(kind=8)
# .MEM_3750 = VDEF <.MEM_3287>
my_gauge = D.14057_1326->r;

/data03/vondele/clean/cp2k/makefiles/../src/qs_linres_current.F:614:0: internal compiler error: verify_stmts failed
Please submit a full bug report.

Not sure how to get a testcase for this one....

It appears to be for example this pointer assignment

my_gauge=>rs_gauge(1)%rs(igrid_level)%rs_grid%r

where 

REAL(dp), DIMENSION(:, :, :), POINTER    ::  my_gauge

and r is:

  TYPE realspace_grid_type

     REAL(KIND=dp), DIMENSION ( :, :, : ), ALLOCATABLE :: r   ! the grid

  END TYPE realspace_grid_type
Comment 1 Joost VandeVondele 2010-09-07 19:28:54 UTC
Simple testcase (gfortran -flto test.f90):

MODULE M1
  INTEGER, PARAMETER :: dp=8
  TYPE realspace_grid_type

     REAL(KIND=dp), DIMENSION ( :, :, : ), ALLOCATABLE :: r

  END TYPE realspace_grid_type
END MODULE

MODULE M2
 USE m1
CONTAINS
 SUBROUTINE S1(x)
  TYPE(realspace_grid_type), POINTER :: x
  REAL(dp), DIMENSION(:, :, :), POINTER    :: y
  y=>x%r
  y=0

 END SUBROUTINE
END MODULE

USE M2
  TYPE(realspace_grid_type), POINTER :: x
  ALLOCATE(x)
  ALLOCATE(x%r(10,10,10))
  CALL S1(x)
  write(6,*) x%r
END
Comment 2 Joost VandeVondele 2010-09-07 19:30:23 UTC
Actually works with 4.5 but fails with trunk
Comment 3 Richard Biener 2010-09-08 09:36:18 UTC
Confirmed.  We have two different array3_real(kind=8) record types that are
not considered compatible.  One data pointer member is restrict qualified
while the other one is not.

Why do we have an aggregate assignment of an array descriptor anyway?

  struct array3_real(kind=8) y;
  struct realspace_grid_type * D.2157;

<bb 2>:
  y.data = 0B;
  D.2157_5 = *x_4(D);
  y = D.2157_5->r;
  D.2172_6 = y.data;

The issue here is of course that LTO re-computes TYPE_CANONICAL and the FE
sets it in a way that the above situation is not detected as non-trivial
conversion.
Comment 4 Joost VandeVondele 2010-09-15 18:25:47 UTC
If realspace_grid_type%r is declared as pointer instead of allocatable, the testcase can be compiled, and a similar workaround in CP2K allows an LTO build of CP2K to finish (after updating binutils to its CVS version).
Comment 5 Joost VandeVondele 2010-09-24 10:33:13 UTC
(In reply to comment #3)
> The issue here is of course that LTO re-computes TYPE_CANONICAL and the FE
> sets it in a way that the above situation is not detected as non-trivial
> conversion.

Does the fix need to be done in the FE, or will this be addressed by changes to LTO?

Actually, looks like there might be some vaguely related issue here in the FE, which I'll open in another PR.
Comment 6 Joost VandeVondele 2010-09-24 10:46:08 UTC
(In reply to comment #5)
> Actually, looks like there might be some vaguely related issue here in the FE,
> which I'll open in another PR.

See PR45777
Comment 7 Richard Biener 2010-09-30 11:49:44 UTC
Mine.
Comment 8 Richard Biener 2010-10-21 15:28:01 UTC
Created attachment 22107 [details]
patch

Bootstrapped, tested and SPEC CPU 2006 tested.

I don't like it too much (it's really a FE hack we put in place that haunts
us now ...).

Deferred installing to stage3 or later (--enable-checking=release will
get you an equivalent solution).
Comment 9 Richard Biener 2010-11-25 16:47:00 UTC
The issue is that realspace_grid_type%r is marked as restrict, but y is
a POINTER and y => x%r associates y with that allocatable even though it
doesn't have TARGET attribute.

This points at either a FE bug, an invalid testcase.  The FE bug would
be that allocatables are not really restrict as they can be aliased.
The invalid testcase would be that doing y => x%r isn't valid (but
gfortran doesn't diagnose that, so also a FE bug).

Can someone clarify why or if it is legal to associate y with x%r?
If declaring x with POINTER attribute makes it legal then we need to
form a type variant for realspace_grid_type, re-using the original
type isn't conformant with middle-end expectations.

FE again for now, atm I think this is a possible generic wrong-code problem.
Comment 10 Richard Biener 2010-11-25 16:50:50 UTC
Thus, isn't what the program does equivalent to

  REAL(dp), DIMENSION(:, :, :), ALLOCATABLE :: z
  REAL(dp), DIMENSION(:, :, :), POINTER    :: y
  y=>z

which is invalid?  Valid would only be

  REAL(dp), DIMENSION(:, :, :), ALLOCATABLE, TARGET :: z
  REAL(dp), DIMENSION(:, :, :), POINTER    :: y
  y=>z

?
Comment 11 Joost VandeVondele 2010-11-25 17:00:19 UTC
(In reply to comment #10)
> Thus, isn't what the program does equivalent to
> 
>   REAL(dp), DIMENSION(:, :, :), ALLOCATABLE :: z
>   REAL(dp), DIMENSION(:, :, :), POINTER    :: y
>   y=>z
> 
> which is invalid?  Valid would only be
> 
>   REAL(dp), DIMENSION(:, :, :), ALLOCATABLE, TARGET :: z
>   REAL(dp), DIMENSION(:, :, :), POINTER    :: y
>   y=>z
> 
> ?

no, your example here is different, and is not allowed. The original testcase is fine. 

so y=>a%b%c%d%z

is allowed as soon as any of a, b, c, or d or z  have the pointer attribute. That z is allocatable doesn't matter.
Comment 12 Michael Matz 2010-11-25 17:02:19 UTC
The following needs to be taken into account when determining the
validity:
If this use is supposed to be valid (we can associate a pointer with
and entity that isn't marked TARGET but merely ALLOCATABLE) then the
distinction between TARGET and ALLOCATABLE (the the language designer
introduced most probably for a reason) is more or less mood.  GemsFDTD
and other benchmarks will regress by a large amount.

So, this testcase should better be invalid due to the missing TARGET
attribute.
Comment 13 Michael Matz 2010-11-25 17:07:10 UTC
> no, your example here is different, and is not allowed. The original
> testcase is fine. 
> 
> so y=>a%b%c%d%z
> 
> is allowed as soon as any of a, b, c, or d or z  have the pointer attribute.
> That z is allocatable doesn't matter.

Ugh.  That might be terrible.  Can you point to some language in the standard
supporting that (I haven't looked myself and am not too fluent in the fortran
standard).
Comment 14 Joost VandeVondele 2010-11-26 10:08:11 UTC
(In reply to comment #13)
> Ugh.  That might be terrible.  Can you point to some language in the standard
> supporting that (I haven't looked myself and am not too fluent in the fortran
> standard).

I'm afraid that finding this in the standard is not quite easy. I guess the idea is that you point to something which is a subobject of variable with pointer attribute. Maybe Tobias can find this? 

The closest I come is actually in IBM's compiler manual:

http://publib.boulder.ibm.com/infocenter/cellcomp/v9v111/index.jsp?topic=/com.ibm.xlf111.cell.doc/xlflr/f90pass.htm

target
    is a variable or expression. If it is a variable, it must have the TARGET attribute (or be a subobject of such an object) or the POINTER attribute.

It isn't that terrible, I think. you still can't point to allocatable arrays, if e.g. they are local variables, or just part of derived types that do not have the pointer attribute.
Comment 15 Richard Biener 2010-11-26 11:32:53 UTC
You then need to make sure to create variant types of aggregates with the
target attribute applied to all subtypes (thus, the restrict stuff removed)
as the middle-end doesn't know about this rule.
Comment 16 Joost VandeVondele 2010-11-26 12:28:20 UTC
I think this implies this bug depends in somehow on PR45777.
Comment 17 Richard Biener 2010-11-26 12:49:39 UTC
(In reply to comment #16)
> I think this implies this bug depends in somehow on PR45777.

Yes indeed - that bug looks very much related.
Comment 18 Tobias Burnus 2010-11-26 13:23:59 UTC
(In reply to comment #13)
> Ugh.  That might be terrible.  Can you point to some language in the standard
> supporting that (I haven't looked myself and am not too fluent in the fortran
> standard).

(All relative to Fortran 2008, ftp://ftp.nag.co.uk/sc22wg5/N1801-N1850/N1830.pdf)

"5.3.17 TARGET attribute"
[...]
"If an object has the TARGET attribute, then all of its nonpointer subobjects also have the TARGET attribute."

The pointer part probably enters indirectly: Assume in "a%b%p%d", "p" is the pointer than "a%b%p%d" denotes the "d" components of the target to which "a%b%p" points to. Per construction the target of the pointer has the TARGET attribute - and "d" has it via the rule above.


Note, however, that the TARGET issue is local. Example:

type t
  integer :: i
end type t
type(t) :: a
integer, pointer :: ptr

call sub(a)
ptr = 7 ! Invalid, ptr has unknown association
        ! status as "a" is not a TARGET

contains

  subroutine sub(x)
    type(a), TARGET :: x
    ptr => x%i ! OK, x and thus x%i is a TARGET
    ptr = 5
  end subroutine
end

Reason: "If the dummy argument has the TARGET attribute and the effective argument does not have the TARGET attribute [...], any pointers associated with the dummy argument become undefined when execution of the procedure completes." (Cf. "12.5.2.4 Ordinary dummy variables".)

See also: "16.5.2.5 Events that cause the association status of pointers to become undefined".


(In reply to comment #15)
> You then need to make sure to create variant types of aggregates with the
> target attribute applied to all subtypes (thus, the restrict stuff removed)
> as the middle-end doesn't know about this rule.

Yes, that sounds sensible - even though it will be a lot of work and requires quite some book keeping :-(

That's somehow the curse of -fwhole-file: Now the decls are sensible enough that the ME has plenty of opportunities to exploit all the wrong decl the FE generates...


(In reply to comment #16)
> I think this implies this bug depends in somehow on PR45777.

Maybe. One operates on gfc_expr level - the other on tree level; however, both are about the same issue. I assume they independent of each other.
Comment 19 Joost VandeVondele 2010-11-26 13:39:00 UTC
Tobias, thanks for the clean explanation. I overlooked that the target of a pointer has that target attribute (seems logical!).

Richard, I tried to get to a testcase for which the ME generates wrong code, but somehow things are always 'right'. I was expecting this to fail (-O3 -fno-inline), since the ME should not now that X aliases with V1%D:

MODULE M1
 IMPLICIT NONE
 TYPE T1
   REAL, DIMENSION(:), ALLOCATABLE :: D
 END TYPE T1
CONTAINS
 SUBROUTINE S1(V1)
   TYPE(T1), POINTER :: V1
   REAL, DIMENSION(:), POINTER :: X
   INTEGER :: I
   CALL PA(V1,X)
   DO i=1,4
      V1%D(i)=1
      X(i)=2
      V1%D(i)=2*V1%D(i)
   ENDDO
 END SUBROUTINE S1
 SUBROUTINE PA(V1,X)
   TYPE(T1), POINTER :: V1
   REAL, DIMENSION(:), POINTER :: X
   X=>V1%D
 END SUBROUTINE
END MODULE M1

USE M1
IMPLICIT NONE
TYPE(T1), POINTER :: V1
INTEGER :: i
ALLOCATE(V1)
ALLOCATE(V1%D(4))
V1%D=(/(i,i=1,4)/)
CALL S1(V1)
IF (ANY(V1%D.NE.4)) STOP "BUG"
write(6,*) "ALL IS FINE"
END
Comment 20 rguenther@suse.de 2010-11-26 14:29:41 UTC
On Fri, 26 Nov 2010, Joost.VandeVondele at pci dot uzh.ch wrote:

> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45586
> 
> --- Comment #19 from Joost VandeVondele <Joost.VandeVondele at pci dot uzh.ch> 2010-11-26 13:39:00 UTC ---
> Tobias, thanks for the clean explanation. I overlooked that the target of a
> pointer has that target attribute (seems logical!).
> 
> Richard, I tried to get to a testcase for which the ME generates wrong code,
> but somehow things are always 'right'. I was expecting this to fail (-O3
> -fno-inline), since the ME should not now that X aliases with V1%D:
> 
> MODULE M1
>  IMPLICIT NONE
>  TYPE T1
>    REAL, DIMENSION(:), ALLOCATABLE :: D
>  END TYPE T1
> CONTAINS
>  SUBROUTINE S1(V1)
>    TYPE(T1), POINTER :: V1

We can't exploit restrict information for pointers, it would need to
be a var of type t1 with target attribute - but even then the double
indirection (v1 passed by reference, embedded array desc points to
data) prevents us from exploiting restrict info.

So we might be safe for now (apart from the related ICE seen with lto).

>    REAL, DIMENSION(:), POINTER :: X
>    INTEGER :: I
>    CALL PA(V1,X)
>    DO i=1,4
>       V1%D(i)=1
>       X(i)=2
>       V1%D(i)=2*V1%D(i)
>    ENDDO
>  END SUBROUTINE S1
>  SUBROUTINE PA(V1,X)
>    TYPE(T1), POINTER :: V1
>    REAL, DIMENSION(:), POINTER :: X
>    X=>V1%D
>  END SUBROUTINE
> END MODULE M1
> 
> USE M1
> IMPLICIT NONE
> TYPE(T1), POINTER :: V1
> INTEGER :: i
> ALLOCATE(V1)
> ALLOCATE(V1%D(4))
> V1%D=(/(i,i=1,4)/)
> CALL S1(V1)
> IF (ANY(V1%D.NE.4)) STOP "BUG"
> write(6,*) "ALL IS FINE"
> END
> 
>
Comment 21 Richard Biener 2010-11-26 14:36:02 UTC
(In reply to comment #20)
> On Fri, 26 Nov 2010, Joost.VandeVondele at pci dot uzh.ch wrote:
> 
> > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45586
> > 
> > --- Comment #19 from Joost VandeVondele <Joost.VandeVondele at pci dot uzh.ch> 2010-11-26 13:39:00 UTC ---
> > Tobias, thanks for the clean explanation. I overlooked that the target of a
> > pointer has that target attribute (seems logical!).
> > 
> > Richard, I tried to get to a testcase for which the ME generates wrong code,
> > but somehow things are always 'right'. I was expecting this to fail (-O3
> > -fno-inline), since the ME should not now that X aliases with V1%D:
> > 
> > MODULE M1
> >  IMPLICIT NONE
> >  TYPE T1
> >    REAL, DIMENSION(:), ALLOCATABLE :: D
> >  END TYPE T1
> > CONTAINS
> >  SUBROUTINE S1(V1)
> >    TYPE(T1), POINTER :: V1
> 
> We can't exploit restrict information for pointers, it would need to
> be a var of type t1 with target attribute - but even then the double
> indirection (v1 passed by reference, embedded array desc points to
> data) prevents us from exploiting restrict info.
> 
> So we might be safe for now (apart from the related ICE seen with lto).

Actually we _can_ see the restrict, but only with -fipa-pta which then
also sees the pointer association (even with -fno-inline) which then
makes the alias visible again (so we ignore restrict, maybe by pure luck
of course):

  D.1625_4 = *v1_2(D);
  # PT = nonlocal unit-escaped null { D.1644 D.1645 } (restr)
  D.1626_5 = D.1625_4->d.data;

  # PT = nonlocal unit-escaped null { D.1644 D.1645 } (restr)
  D.1630_10 = x.data;

wonders of propagation ;)
Comment 22 Tobias Burnus 2011-01-13 15:23:29 UTC
Unassign Richard, who stated that he plans to ignore the issue for 4.6.

The proper solution seems to be to fix it on the FE side - though I have not not fully re-read this PR to know whether it is possible or whether something in the definition of the Fortran standard prevents it and still requires some middle-end help/solution/hack.

A work around is build with release checking (which is the default on the release branches).
Comment 23 Thomas Koenig 2011-01-16 12:02:57 UTC
We could do the analysis that fixed PR 45777 here, I just don't know
where :-)

Could somebody point me to the right place (I presume somewhere in trans-*)?
Comment 24 Mikael Morin 2011-01-16 14:25:05 UTC
(In reply to comment #23)
> We could do the analysis that fixed PR 45777 here, I just don't know
> where :-)
> 
> Could somebody point me to the right place (I presume somewhere in trans-*)?

$ grep RESTRICT *
ChangeLog-2009:	instead set DECL_RESTRICTED_P on affected decls.
trans-decl.c:    DECL_RESTRICTED_P (decl) = 1;
trans-types.c:  prvoid_type_node = build_qualified_type (pvoid_type_node, TYPE_QUAL_RESTRICT);
trans-types.c:			    TYPE_QUAL_RESTRICT);
trans-types.c:	type = build_qualified_type (type, TYPE_QUAL_RESTRICT);
trans-types.c:    arraytype = build_qualified_type (arraytype, TYPE_QUAL_RESTRICT);
trans-types.c:	    type = build_qualified_type (type, TYPE_QUAL_RESTRICT);
$

then if you look at those places, they are guarded with a "if (restricted)" condition, so the places you are looking for are those of calls to functions expecting an argument named restricted (gfc_build_array_type, gfc_get_nodesc_array_type). 

However, it doesn't look that easy. 
If I understand the thing correctly, in case you have an entity with a pointer or target attribute, you have to build a variant of its base type without the restrict attribute. That seems easy. Less easy is that you have to build a variant type without the restrict attribute for each of it's sub-components too. 

That is, for
!!!!!
type foo
  integer, allocatable :: bar(:)
end type foo
type(foo) :: x
type(foo), pointer :: y

call baz (x, y)
!!!!!
The type of baz's first actual argument (namely &x) is struct foo * restrict
but the type of baz's second actual argument (namely &y) is not struct foo *.

typeof(&x) ==
struct foo_restrict { 
  struct integer_array {
    struct array_bounds {
      int lbound, ubound, stride;
    } bounds [];
    ...
    integer * restrict data;
  } bar;
} * restrict

whereas

typeof(&y) ==
struct foo_norestrict { 
  struct integer_array {
    struct array_bounds {
      int lbound, ubound, stride;
    } bounds [];
    ...
    integer * data;
  } bar;
} *


Note that the data member has lost the restrict too. Thus, when creating such pointer qualified types, one should take care not to update the sym->backend_decl of the derived type as they are different. That looks like a substantial rework. 
Or maybe drop restrict all together on the type for correctness.

By the way, does the middle-end support creating a variant of a type with a sub-component restrict (un)qualified ?
Comment 25 tkoenig@netcologne.de 2011-01-16 16:23:29 UTC
Maybe it would be better to set the "inherited" pointer and target
attributes much earlier, in gfc_variable_attr.  With a bit of luck,
things would then work automatically.
Comment 26 Richard Biener 2011-01-17 10:08:13 UTC
(In reply to comment #24)
> (In reply to comment #23)
> > We could do the analysis that fixed PR 45777 here, I just don't know
> > where :-)
> > 
> > Could somebody point me to the right place (I presume somewhere in trans-*)?
> 
> $ grep RESTRICT *
> ChangeLog-2009:    instead set DECL_RESTRICTED_P on affected decls.
> trans-decl.c:    DECL_RESTRICTED_P (decl) = 1;
> trans-types.c:  prvoid_type_node = build_qualified_type (pvoid_type_node,
> TYPE_QUAL_RESTRICT);
> trans-types.c:                TYPE_QUAL_RESTRICT);
> trans-types.c:    type = build_qualified_type (type, TYPE_QUAL_RESTRICT);
> trans-types.c:    arraytype = build_qualified_type (arraytype,
> TYPE_QUAL_RESTRICT);
> trans-types.c:        type = build_qualified_type (type, TYPE_QUAL_RESTRICT);
> $
> 
> then if you look at those places, they are guarded with a "if (restricted)"
> condition, so the places you are looking for are those of calls to functions
> expecting an argument named restricted (gfc_build_array_type,
> gfc_get_nodesc_array_type). 
> 
> However, it doesn't look that easy. 
> If I understand the thing correctly, in case you have an entity with a pointer
> or target attribute, you have to build a variant of its base type without the
> restrict attribute. That seems easy. Less easy is that you have to build a
> variant type without the restrict attribute for each of it's sub-components
> too. 
> 
> That is, for
> !!!!!
> type foo
>   integer, allocatable :: bar(:)
> end type foo
> type(foo) :: x
> type(foo), pointer :: y
> 
> call baz (x, y)
> !!!!!
> The type of baz's first actual argument (namely &x) is struct foo * restrict
> but the type of baz's second actual argument (namely &y) is not struct foo *.
> 
> typeof(&x) ==
> struct foo_restrict { 
>   struct integer_array {
>     struct array_bounds {
>       int lbound, ubound, stride;
>     } bounds [];
>     ...
>     integer * restrict data;
>   } bar;
> } * restrict
> 
> whereas
> 
> typeof(&y) ==
> struct foo_norestrict { 
>   struct integer_array {
>     struct array_bounds {
>       int lbound, ubound, stride;
>     } bounds [];
>     ...
>     integer * data;
>   } bar;
> } *
> 
> 
> Note that the data member has lost the restrict too. Thus, when creating such
> pointer qualified types, one should take care not to update the
> sym->backend_decl of the derived type as they are different. That looks like a
> substantial rework. 
> Or maybe drop restrict all together on the type for correctness.

Indeed when we introduced the restrict handling to the Fortran frontend
we thought that the above case would not happen, thus that the testcase
in question would be not a valid Fortran program.  At the moment
we probably even could generate a testcase that generates wrong-code
(when the ICE silences itself with release-checking).  This means that
simply trying to make it not ICE is probably wrong and simply hides
the real issue (which the ICE shows).

It is, btw, a sign of bad Fortran language design and makes the point
of the pointer/target attributes moot.

What we could do is drop restrict qualifications for all aggregate
types.  At least such funny games do not seem possible with mere
toplevel arrays or scalars, right?

> By the way, does the middle-end support creating a variant of a type with a
> sub-component restrict (un)qualified ?

No.  For type variants all the fields are usually shared, you'd have to
create a deep copy.
Comment 27 Mikael Morin 2011-01-17 13:01:23 UTC
(In reply to comment #25)
> Maybe it would be better to set the "inherited" pointer and target
> attributes much earlier, in gfc_variable_attr.  With a bit of luck,
> things would then work automatically.
If you mean set the gfc_component.attr.target field, I don't think it's right. 
As gfc_component structs belong to the type's gfc_symbol, they are shared between non-pointer and pointer variants. 

(In reply to comment #26)
> (In reply to comment #24)
> > Or maybe drop restrict all together on the type for correctness.
> 
> Indeed when we introduced the restrict handling to the Fortran frontend
> we thought that the above case would not happen, thus that the testcase
> in question would be not a valid Fortran program.  At the moment
> we probably even could generate a testcase that generates wrong-code
> (when the ICE silences itself with release-checking).  This means that
> simply trying to make it not ICE is probably wrong and simply hides
> the real issue (which the ICE shows).
> 
> It is, btw, a sign of bad Fortran language design and makes the point
> of the pointer/target attributes moot.
I think it makes some sense:
If there is more than one data path to a struct, there is more than one data path to all of its components. Thus the inherited target attribute.
It's just it doesn't fit the C-minded middle-end well. :-(

> 
> What we could do is drop restrict qualifications for all aggregate
> types.  At least such funny games do not seem possible with mere
> toplevel arrays or scalars, right?
Well, i think so. 


> 
> > By the way, does the middle-end support creating a variant of a type with a
> > sub-component restrict (un)qualified ?
> 
> No.  For type variants all the fields are usually shared, you'd have to
> create a deep copy.

Which means they are not type-compatible anymore ?
Which means casts everywhere ?
Comment 28 Tobias Burnus 2011-01-17 13:20:42 UTC
(In reply to comment #26)
> It is, btw, a sign of bad Fortran language design and makes the point
> of the pointer/target attributes moot.

Well, I think it is just different model, which is used by the middle end and by the Fortran language, thus there is no simple one-to-one matching. That a hack was needed to get a better "restricted" support is already a hint that the models are a bit incompatible.

Fortran's handling is simple: Every variable does not alias by default - the compiler only has to assume aliasing (of the variable and its components) if the variable is explicitly marked as TARGET in that scope; for sections of code where it does not have a TARGET attribute, the user has to ensure that there is no aliasing. There is only one complication: pointers and pointer components of derived types ("TYPE") can alias. [Cf. comment 18]

Seemingly there is an issue of propagating this information properly to the middle end.

Thus, for the first test case (comment 1):

  TYPE realspace_grid_type
     REAL(KIND=dp), DIMENSION ( :, :, : ), ALLOCATABLE :: r
  END TYPE realspace_grid_type
  TYPE(realspace_grid_type), POINTER :: x, y

The "POINTER" attribute means that "x" and "y" are pointers; thus, if "x" and "y" are the same, x%r and y%r are the same, if "x" and "y" are different, "x%r" and "y%r" are different arrays. Additionally, the "pointer" attribute means that "x%r" and "y%r" have implicitly the TARGET attribute.


Quoting comment 15:
> You then need to make sure to create variant types of aggregates with the
> target attribute applied to all subtypes (thus, the restrict stuff removed)
> as the middle-end doesn't know about this rule.

Which seems to be sensible - though it might need a substantial rework of the FE to keep all the time both variants available:

From comment 24:
> Note that the data member has lost the restrict too. Thus, when creating such
> pointer qualified types, one should take care not to update the
> sym->backend_decl of the derived type as they are different. That looks
> like a substantial rework.

One way would be to keep for data types all the time the two versions around:
One with restrict and one without restrict; thus, if one does type extension, one has always a fully restrictless data type. Unfortunately, that seems to cause a missed-optimization for
  type t
    integer :: a
    integer, pointer :: b
  end type t
  type(t) :: x
as "x%a" cannot alias and only "x%b" can - but is seems that one cannot encode this for the ME such that "t" as a whole has to be marked as unrestricted.


> Or maybe drop restrict all together on the type for correctness.

Well, "restrict" is a very important means to allow for optimization; at least as long as no POINTER is involved (neither the data type nor a component) and also no TARGET, one should really use restricted pointers. For the other cases, one has probably to accept that the ME does not support Fortran's rules :-(

I think the easiest would be to create for derived types sym->backend_decl and another backend_decl with "restrict" - and propagate both variants types through, when extending the type or including it into another type. As soon as it is used as POINTER component, one could drop propagating the backend_decl_restricted.
Comment 29 Michael Matz 2011-01-17 13:52:20 UTC
> It is, btw, a sign of bad Fortran language design

I don't see that.  The frontend merely needs to emit correctly typed
expressions.  And that the type of 'a%b%ptr%d' depends on the prefix
'a%b%ptr' is no wonder and happens with most languages.

> and makes the point
> of the pointer/target attributes moot.

Not really.  The point being that non-pointer objects themself can't refer to
any other object, and that non-target objects can't be referred to by
anything else than their name or dummies.  So, as long as no pointer
is involved in an expression all is well.  And as soon as a pointer is
involved everything works automatically iff the pointer variable has the
correct type (namely a type with an implied target attribute everywhere,
which in frontend parlance is equivalent to a type without any restrict
markers applied).

> What we could do is drop restrict qualifications for all aggregate
> types.  At least such funny games do not seem possible with mere
> toplevel arrays or scalars, right?

IIRC the spec2k6 cases for which I added the restrict support were
using aggregate types :-/
Comment 30 Mikael Morin 2011-01-18 12:48:41 UTC
(In reply to comment #28)
> One way would be to keep for data types all the time the two versions around:
> One with restrict and one without restrict;
makes sense

> thus, if one does type extension,
> one has always a fully restrictless data type.
Why? From my point of view type extension is orthogonal to target. 
If an entity of extended derived type has the target/pointer attribute, one uses the non-restrict variant for its base type, and if it hasn't one uses the restrict variant. Do I miss something?

> Unfortunately, that seems to
> cause a missed-optimization for
>   type t
>     integer :: a
>     integer, pointer :: b
>   end type t
>   type(t) :: x
> as "x%a" cannot alias and only "x%b" can - but is seems that one cannot encode
> this for the ME such that "t" as a whole has to be marked as unrestricted.

How about: 

struct t_restrict {
  int a;
  int * b;
} * restrict

VS

struct t_norestrict {
  int a;
  int * b;
} *

?


> 
> I think the easiest would be to create for derived types sym->backend_decl and
> another backend_decl with "restrict" - and propagate both variants types
> through, when extending the type or including it into another type. As soon as
> it is used as POINTER component, one could drop propagating the
> backend_decl_restricted.

Looks like a plan. :-)
Comment 31 rguenther@suse.de 2011-01-18 13:37:42 UTC
On Tue, 18 Jan 2011, mikael at gcc dot gnu.org wrote:

> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45586
> 
> --- Comment #30 from Mikael Morin <mikael at gcc dot gnu.org> 2011-01-18 12:48:41 UTC ---
> (In reply to comment #28)
> > One way would be to keep for data types all the time the two versions around:
> > One with restrict and one without restrict;
> makes sense
> 
> > thus, if one does type extension,
> > one has always a fully restrictless data type.
> Why? From my point of view type extension is orthogonal to target. 
> If an entity of extended derived type has the target/pointer attribute, one
> uses the non-restrict variant for its base type, and if it hasn't one uses the
> restrict variant. Do I miss something?
> 
> > Unfortunately, that seems to
> > cause a missed-optimization for
> >   type t
> >     integer :: a
> >     integer, pointer :: b
> >   end type t
> >   type(t) :: x
> > as "x%a" cannot alias and only "x%b" can - but is seems that one cannot encode
> > this for the ME such that "t" as a whole has to be marked as unrestricted.
> 
> How about: 
> 
> struct t_restrict {
>   int a;
>   int * b;
> } * restrict
> 
> VS
> 
> struct t_norestrict {
>   int a;
>   int * b;
> } *
> 
> ?

Yes.  The restrict qualifier on t_restrict * does not apply
to members, so as long as int *b is not restrict qualified
the b's alias.

> > I think the easiest would be to create for derived types sym->backend_decl and
> > another backend_decl with "restrict" - and propagate both variants types
> > through, when extending the type or including it into another type. As soon as
> > it is used as POINTER component, one could drop propagating the
> > backend_decl_restricted.
> 
> Looks like a plan. :-)

I think Micha has started working on something.

Richard.
Comment 32 Michael Matz 2011-01-18 13:56:01 UTC
Yes, but it's possible I was going up the wrong tree.  My idea was to
build the no-restrict variants of types on demand, as necessary, basically
from gfc_sym_type(), whenever sym->attr.pointer.

This nearly worked, as in, it gives the top-level objects ('x' in question
of the testcase from comment #2) a correct type (a record of only non-restrict
members, recursively).

Unfortunately the fortran frontend uses a peculiar way to expand the actual
'x%r' construct.  The necessary FIELD_DECL is not pulled out from the
type of 'x' (I mean the TREE_TYPE), but rather from the frontend-specific
representation of types, in this case from the r-named-component's
backend_decl.  And that very backend_decl itself doesn't depend on the
context, as in, there's always only one for a given type.

So, yes, adding another backend_decl for one component which would hold
the no-restrict variant would work.  But that feels like a gross hack.

Everything would work iff 'x' had the proper type from the start, in
the frontend representation already.  Then its components also would have
the proper attributes set (target namely), leading to correctly typed
backend_decls automatically.

I quickly tried to implement something like that last paragraph,
but were unsuccessful so far.  My basic idea was to remember if a frontend
decl had attr.pointer set, and then set attr.target for all subcomponents
(via current_attr or otherwise).  I thought this was possible as it seemed
to me that the frontend possibly builds types for declarations anew each
time if necessary.  But I might be wrong.
Comment 33 Mikael Morin 2011-01-18 17:21:54 UTC
(In reply to comment #32)
> Yes, but it's possible I was going up the wrong tree.  My idea was to
> build the no-restrict variants of types on demand, as necessary, basically
> from gfc_sym_type(), whenever sym->attr.pointer.
> 
> This nearly worked, as in, it gives the top-level objects ('x' in question
> of the testcase from comment #2) a correct type (a record of only non-restrict
> members, recursively).
> 
> Unfortunately the fortran frontend uses a peculiar way to expand the actual
> 'x%r' construct.  The necessary FIELD_DECL is not pulled out from the
> type of 'x' (I mean the TREE_TYPE), but rather from the frontend-specific
> representation of types, in this case from the r-named-component's
> backend_decl.  And that very backend_decl itself doesn't depend on the
> context, as in, there's always only one for a given type.
> 
> So, yes, adding another backend_decl for one component which would hold
> the no-restrict variant would work.  But that feels like a gross hack.
> 
> Everything would work iff 'x' had the proper type from the start, in
> the frontend representation already.  Then its components also would have
> the proper attributes set (target namely), leading to correctly typed
> backend_decls automatically.
> 
> I quickly tried to implement something like that last paragraph,
> but were unsuccessful so far.  My basic idea was to remember if a frontend
> decl had attr.pointer set, and then set attr.target for all subcomponents
> (via current_attr or otherwise).  I thought this was possible as it seemed
> to me that the frontend possibly builds types for declarations anew each
> time if necessary.  But I might be wrong.
As I said in comment #27, gfc_component structs belonging to the type, they are shared between target and non-target variants. Thus one cannot set inherited target attributes on them. 
Consider this :
!!!!!!!!!!!!!
type bar
  integer :: a
end type bar

type(bar), pointer :: b
type(bar)          :: c
end
!!!!!!!!!!!!!
If you want to set the target attribute on b%a, you will set it on the `a' component of the `bar' type, and will impact `c' as well.
Comment 34 Michael Matz 2011-01-19 16:39:30 UTC
> As I said in comment #27, gfc_component structs belonging to the type, they
> are shared between target and non-target variants. Thus one cannot set
> inherited target attributes on them.

Yep, that's what I figured eventually :)  The question now is if for:

--------------------------
type bar
  integer :: a
  end type bar
  type(bar), pointer :: b
  type(bar)          :: c
--------------------------

'b' and 'c' should have the same type.  In other words
should "type(bar),pointer" be a different type vs "type(bar)" already in the
frontend representation for types, or not?  If they would be they wouldn't
have shared components and we would be free to set attributes as we like.

Not knowing much about the several engineering decisions taken while
developing the frontend _I_ personally would have done it this way.  Alas,
no cake for me :)

So, some guidance by the frontend maintainers would be welcome which path
I should take.  Making it more the above suggestion, or really going
with the two backend_decls per entity?  (what I'm worried about with the
two backend_decls is, what in one year we find other reasons why some
attributes need to be different from the base case, then we already need
four backend_delcs)
Comment 35 Michael Matz 2011-01-20 16:36:23 UTC
Created attachment 23047 [details]
possible patch

So, this is my current version.  I'm creating a different type for top-level
symbols that are attr.pointer.  Further I'm checking the accessed FIELD_DECLs
for compatibility with the parent type.  If they are not, I explicitely
search the parent type for a same-named FIELD_DECL (which by construction
must exist and match).  I cache this result in a new field of components.
In this way we could theoretically also support a number of other different
record types without having to add new backend_decl fields for each
combination (we would merely add a linear number of more cache entries,
in the hope that the program author is not in fact making use of all such
combinations).

It fixes the testcase.  Haven't tested much further.
Comment 36 Mikael Morin 2011-01-21 22:54:53 UTC
(In reply to comment #34)
> Yep, that's what I figured eventually :)  The question now is if for:
> 
> --------------------------
> type bar
>   integer :: a
>   end type bar
>   type(bar), pointer :: b
>   type(bar)          :: c
> --------------------------
> 
> 'b' and 'c' should have the same type.  In other words
> should "type(bar),pointer" be a different type vs "type(bar)" already in the
> frontend representation for types, or not?  If they would be they wouldn't
> have shared components and we would be free to set attributes as we like.
> 
> Not knowing much about the several engineering decisions taken while
> developing the frontend _I_ personally would have done it this way.  Alas,
> no cake for me :)

Well, the current scheme is in line with how the standard is meant: entities of the same type can have or not the target attribute; in other words attributes are orthogonal to the type. This doesn't fit the middle-end but having it fit the middle-end would lead to the reverse problem : every time we find two objects of different type, we would have to look for variants to check if the objects are indeed of different type or only of different variants (and thus compatible). 
Personally, I would put the fix in the middle-end (which doesn't mean there is nothing to do on the front-end side). Instead of having the C type-compatibility rules hardcoded in the middle-end, have some relaxed rules:  two objects are type-compatible iff all of their sub-components are type compatible. I guess no one wants to see the hack in his own area. 

> 
> So, some guidance by the frontend maintainers would be welcome which path
> I should take.  Making it more the above suggestion, or really going
> with the two backend_decls per entity?  (what I'm worried about with the
> two backend_decls is, what in one year we find other reasons why some
> attributes need to be different from the base case, then we already need
> four backend_delcs)

That's a valid concern. 
The following attributes are recursive like TARGET:
ASYNCHRONOUS, INTENT, PROTECTED, VOLATILE
Of those, INTENT and VOLATILE appear in the middle-end type (and TARGET of course). 
For ASYNCHRONOUS I don't know.



(In reply to comment #35)
> Created attachment 23047 [details]
> possible patch
> 
> So, this is my current version.  I'm creating a different type for top-level
> symbols that are attr.pointer.  Further I'm checking the accessed FIELD_DECLs
> for compatibility with the parent type.  If they are not, I explicitely
> search the parent type for a same-named FIELD_DECL (which by construction
> must exist and match).  I cache this result in a new field of components.
> In this way we could theoretically also support a number of other different
> record types without having to add new backend_decl fields for each
> combination (we would merely add a linear number of more cache entries,
> in the hope that the program author is not in fact making use of all such
> combinations).
> 

I'll look at it tomorrow.
Comment 37 rguenther@suse.de 2011-01-24 09:41:28 UTC
On Fri, 21 Jan 2011, mikael at gcc dot gnu.org wrote:

> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45586
> 
> --- Comment #36 from Mikael Morin <mikael at gcc dot gnu.org> 2011-01-21 22:54:53 UTC ---
> (In reply to comment #34)
> > Yep, that's what I figured eventually :)  The question now is if for:
> > 
> > --------------------------
> > type bar
> >   integer :: a
> >   end type bar
> >   type(bar), pointer :: b
> >   type(bar)          :: c
> > --------------------------
> > 
> > 'b' and 'c' should have the same type.  In other words
> > should "type(bar),pointer" be a different type vs "type(bar)" already in the
> > frontend representation for types, or not?  If they would be they wouldn't
> > have shared components and we would be free to set attributes as we like.
> > 
> > Not knowing much about the several engineering decisions taken while
> > developing the frontend _I_ personally would have done it this way.  Alas,
> > no cake for me :)
> 
> Well, the current scheme is in line with how the standard is meant: entities of
> the same type can have or not the target attribute; in other words attributes
> are orthogonal to the type. This doesn't fit the middle-end but having it fit
> the middle-end would lead to the reverse problem : every time we find two
> objects of different type, we would have to look for variants to check if the
> objects are indeed of different type or only of different variants (and thus
> compatible). 
> Personally, I would put the fix in the middle-end (which doesn't mean there is
> nothing to do on the front-end side). Instead of having the C
> type-compatibility rules hardcoded in the middle-end, have some relaxed rules: 
> two objects are type-compatible iff all of their sub-components are type
> compatible.

That's what we do ;)  And void * restrict is not compatible with
void *.  So you get the ICE.

Richard.
Comment 38 Mikael Morin 2011-01-25 14:32:28 UTC
The patch looks good. 
Somewhat hackish as you acknowledge, but worth submitting anyway. 
A few minor comments below. 


> Index: fortran/trans-expr.c
> ===================================================================
> --- fortran/trans-expr.c	(revision 168749)
> +++ fortran/trans-expr.c	(working copy)
> @@ -504,6 +504,19 @@ gfc_conv_component_ref (gfc_se * se, gfc
>    field = c->backend_decl;
>    gcc_assert (TREE_CODE (field) == FIELD_DECL);
>    decl = se->expr;
> +  if (DECL_CONTEXT (field) != TREE_TYPE (decl))
> +    {
> +      tree f2 = c->backend_decl2;
> +      if (f2 && DECL_FIELD_CONTEXT (f2) == TREE_TYPE (decl))
> +	;
> +      else for (f2 = TYPE_FIELDS (TREE_TYPE (decl)); f2; f2 = DECL_CHAIN (f2))

I prefer `if (!cond) ...' instead of `if (cond) ; else ...'

 
> Index: fortran/gfortran.h
> ===================================================================
> --- fortran/gfortran.h	(revision 168749)
> +++ fortran/gfortran.h	(working copy)
> @@ -934,6 +934,7 @@ typedef struct gfc_component
>    gfc_array_spec *as;
>  
>    tree backend_decl;
> +  tree backend_decl2;
More descriptive name (e.g. restrict_backend_decl or target_backend_decl or ...)
and comment explaining the need for it appreciated. 

One could have the combinatorial explosion of backend_decls here as said Michael. In any case it can be safely removed if needed, as it is just a cache.


>    locus loc;
>    struct gfc_expr *initializer;
>    struct gfc_component *next;
> Index: fortran/trans-types.c
> ===================================================================
> --- fortran/trans-types.c	(revision 168749)
> +++ fortran/trans-types.c	(working copy)
> @@ -1746,6 +1746,80 @@ gfc_build_pointer_type (gfc_symbol * sym
>    else
>      return build_pointer_type (type);
>  }
> +
> +static tree
> +gfc_nonrestricted_type (tree t)
> +{
> +  tree ret = t;
> +  if (!TYPE_LANG_SPECIFIC (t))
> +    TYPE_LANG_SPECIFIC (t)
> +      = ggc_alloc_cleared_lang_type (sizeof (struct lang_type));
> +  if (TYPE_LANG_SPECIFIC (t)->nonrestricted_type)
> +    return TYPE_LANG_SPECIFIC (t)->nonrestricted_type;
> +  switch (TREE_CODE (t))
> +    {
> +      default:
> +	break;
> +
> +      case POINTER_TYPE:
> +      case REFERENCE_TYPE:
> +	ret = build_qualified_type (t, TYPE_QUALS (t) & ~TYPE_QUAL_RESTRICT);

Isn't it necessary to call gfc_nonrestricted_type on TREE_TYPE (t) here ?


> +	break;
> +
> +      case ARRAY_TYPE:
> +	{
> +	  tree elemtype = gfc_nonrestricted_type (TREE_TYPE (t));
> +	  if (elemtype == TREE_TYPE (t))
> +	    ret = t;
> +	  else
> +	    {
> +	      ret = copy_node (t);
> +	      TREE_TYPE (t) = elemtype;
> +	      /* ??? Change some TYPE_LANG_SPECIFICs too?  */
> +	    }
> +	}
> +	break;
> +
> +      case RECORD_TYPE:
> +      case UNION_TYPE:
> +      case QUAL_UNION_TYPE:
> +	{
> +	  tree field, *chain;
> +	  for (field = TYPE_FIELDS (t); field; field = DECL_CHAIN (field))
> +	    if (TREE_CODE (field) == FIELD_DECL)
> +	      {
> +		tree elemtype = gfc_nonrestricted_type (TREE_TYPE (field));
> +		if (elemtype != TREE_TYPE (field))
> +		  break;
> +	      }
> +	  if (!field)
> +	    break;
> +	  ret = copy_node (t);
> +	  TYPE_FIELDS (ret) = NULL_TREE;
> +	  chain = &TYPE_FIELDS (ret);
> +	  for (field = TYPE_FIELDS (t); field; field = DECL_CHAIN (field))
> +	    {
> +	      tree newfield = copy_node (field);
> +	      DECL_CONTEXT (newfield) = ret;
> +	      DECL_CHAIN (newfield) = NULL_TREE;
> +	      if (TYPE_FIELDS (ret) == NULL_TREE)
> +		TYPE_FIELDS (ret) = newfield;

Those two lines seem to duplicate the line below (as initially chain points to &TYPE_FIELDS(ret)). 


> +	      *chain = newfield;
> +	      chain = &DECL_CHAIN (newfield);
> +
> +	      if (TREE_CODE (field) == FIELD_DECL)
> +		{
> +		  tree elemtype = gfc_nonrestricted_type (TREE_TYPE (field));
> +		  TREE_TYPE (newfield) = elemtype;
> +		}
> +	    }
> +	}
> +        break;
> +    }
> +  TYPE_LANG_SPECIFIC (t)->nonrestricted_type = ret;

Don't know if it is absolutely necessary, but one might add also :
TYPE_LANG_SPECIFIC (ret)->nonrestricted_type = ret;


> +  return ret;
> +}
> +
>  
>  /* Return the type for a symbol.  Special handling is required for character
>     types to get the correct level of indirection.
> @@ -1789,6 +1863,9 @@ gfc_sym_type (gfc_symbol * sym)
>    else
>      type = gfc_typenode_for_spec (&sym->ts);
>  
> +  if (sym->attr.pointer)
> +    type = gfc_nonrestricted_type (type);
> +

This is missing the target attribute, so you may preferably use the restricted boolean a few lines below. 


>    if (sym->attr.dummy && !sym->attr.function && !sym->attr.value)
>      byref = 1;
>    else
> Index: fortran/trans.h
> ===================================================================
> --- fortran/trans.h	(revision 168749)
> +++ fortran/trans.h	(working copy)
> @@ -700,6 +700,7 @@ struct GTY((variable_size))	lang_type	 {
>    tree dataptr_type;
>    tree span;
>    tree base_decl[2];
> +  tree nonrestricted_type;
>  };
>  
>  struct GTY((variable_size)) lang_decl {
Comment 39 Mikael Morin 2011-01-25 14:40:40 UTC
(In reply to comment #37)
> 
> That's what we do ;) 

Wow! Middle-end gurus take design decisions of mine before I have ever thought them. They are real wizards after all. 


> And void * restrict is not compatible with
> void *.  So you get the ICE.

Hum, may I suggest a --push-harder/--will-you-swallow-it option ?


More seriously, I will test the patch above.
Comment 40 Michael Matz 2011-01-25 15:02:40 UTC
The patch from comment #35 requires another change in unrelated code, which
I think actually fixes a pre-existing bug in type extension support:

Index: fortran/trans-expr.c
===================================================================
--- fortran/trans-expr.c        (revision 168749)
+++ fortran/trans-expr.c        (working copy)
@@ -549,11 +562,11 @@ conv_parent_component_references (gfc_se

   if (dt->attr.extension && dt->components)
     {
-      if (dt->attr.is_class)
+      if (1 || dt->attr.is_class)
        cmp = dt->components;
       else
        cmp = dt->components->next;
-      /* Return if the component is not in the parent type.  */
+      /* Return if the component is in this type.  */
       for (; cmp; cmp = cmp->next)
        if (strcmp (c->name, cmp->name) == 0)
          return;

Otherwise the new assert will trigger on extends_*.f03 because the frontend
is trying to generate obviously wrong component_refs.
Comment 41 Joost VandeVondele 2011-01-25 15:03:55 UTC
(In reply to comment #39)
> > void *.  So you get the ICE.
> Hum, may I suggest a --push-harder/--will-you-swallow-it option ?

--enable-checking=release ?
Comment 42 Mikael Morin 2011-01-25 21:41:00 UTC
(In reply to comment #40)

There is indeed something fishy here. 
Your change does the right thing in the non-class case I think ; can't tell about the class case. 
Janus ?

The code tries to match a reference to an inherited component but misses references to the base types themselves.

In extends_1.f03, for:

new_person%service%education%person%ss = ss

the dump shows:

new_person->service.education.person.service.education.person.education.person.person.ss = *ss;


Despite this, the middle-end manages to produce something runnable. :-)
Comment 43 Michael Matz 2011-01-26 12:39:04 UTC
Yep.  With my patch the saner looking
  new_person->service.education.person.ss = *ss;
statement is generated.  It's possible that class containers actually contain
something as first component that shouldn't be looked up, then the condition
would be reversed, or that meanwhile the parent type is always reachable
via the first component.  The latter is definitely the case for types that
don't have is_class set.
Comment 44 janus 2011-02-07 22:15:47 UTC
(In reply to comment #42)
> (In reply to comment #40)
> 
> There is indeed something fishy here. 
> Your change does the right thing in the non-class case I think ; can't tell
> about the class case. 
> Janus ?

Sorry for the late answer. I think the patch is ok. It does not affect OOP anyway. And for non-class types there was a bug apparently. AFAICS one could simplify 'conv_parent_component_references' in the following way:

Index: gcc/fortran/trans-expr.c
===================================================================
--- gcc/fortran/trans-expr.c    (revision 169891)
+++ gcc/fortran/trans-expr.c    (working copy)
@@ -538,6 +538,11 @@ conv_parent_component_references (gfc_se * se, gfc
   dt = ref->u.c.sym;
   c = ref->u.c.component;
 
+  /* Return if the component is not in the parent type.  */
+  for (cmp = dt->components; cmp; cmp = cmp->next)
+    if (strcmp (c->name, cmp->name) == 0)
+      return;
+  
   /* Build a gfc_ref to recursively call gfc_conv_component_ref.  */
   parent.type = REF_COMPONENT;
   parent.next = NULL;
@@ -546,24 +551,12 @@ conv_parent_component_references (gfc_se * se, gfc
 
   if (dt->backend_decl == NULL)
     gfc_get_derived_type (dt);
-
-  if (dt->attr.extension && dt->components)
-    {
-      if (dt->attr.is_class)
-       cmp = dt->components;
-      else
-       cmp = dt->components->next;
-      /* Return if the component is not in the parent type.  */
-      for (; cmp; cmp = cmp->next)
-       if (strcmp (c->name, cmp->name) == 0)
-         return;
-
-      /* Otherwise build the reference and call self.  */
-      gfc_conv_component_ref (se, &parent);
-      parent.u.c.sym = dt->components->ts.u.derived;
-      parent.u.c.component = c;
-      conv_parent_component_references (se, &parent);
-    }
+    
+  /* Otherwise build the reference and call self.  */
+  gfc_conv_component_ref (se, &parent);
+  parent.u.c.sym = dt->components->ts.u.derived;
+  parent.u.c.component = c;
+  conv_parent_component_references (se, &parent);
 }
 
 /* Return the contents of a variable. Also handles reference/pointer
Comment 45 Tobias Burnus 2011-02-12 13:09:06 UTC
Author: burnus
Date: Sat Feb 12 13:09:03 2011
New Revision: 170072

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=170072
Log:
2011-02-12  Michael Matz  <matz@suse.de>
            Janus Weil  <janus@gcc.gnu.org>
            Tobias Burnus  <burnus@net-b.de>

        PR fortran/45586
        * trans-expr.c (conv_parent_component_references): Avoid
        unintendent skipping of parent compounds.


Modified:
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/trans-expr.c
Comment 46 Tobias Burnus 2011-02-12 13:12:26 UTC
(In reply to comment #45)
> New Revision: 170072

That patch fixed the issue of comment 40 to comment 44.

TODO: The actual restrict patch of comment 35 (attachment 23047 [details]), taking into account the review comment as comment 38
Comment 47 Mikael Morin 2011-02-12 14:56:35 UTC
Author: mikael
Date: Sat Feb 12 14:56:32 2011
New Revision: 170074

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=170074
Log:
2011-02-12  Mikael Morin  <mikael.morin@sfr.fr>

	PR fortran/45586
	* gfortran.dg/extends_11.f03: New.


Added:
    trunk/gcc/testsuite/gfortran.dg/extends_11.f03
Modified:
    trunk/gcc/testsuite/ChangeLog
Comment 48 Michael Matz 2011-02-18 19:52:19 UTC
Author: matz
Date: Fri Feb 18 19:52:16 2011
New Revision: 170284

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=170284
Log:
	PR fortran/45586
	* gfortran.h (struct gfc_component): Add norestrict_decl member.
	* trans.h (struct lang_type): Add nonrestricted_type member.
	* trans-expr.c (gfc_conv_component_ref): Search fields with correct
	parent type.
	* trans-types.c (mirror_fields, gfc_nonrestricted_type): New.
	(gfc_sym_type): Use it.

testsuite/
	PR fortran/45586
	* gfortran.dg/lto/pr45586_0.f90: New test.
	* gfortran.dg/typebound_proc_20.f90: Ditto.
	* gfortran.dg/typebound_proc_21.f90: Ditto.

Added:
    trunk/gcc/testsuite/gfortran.dg/lto/pr45586_0.f90
    trunk/gcc/testsuite/gfortran.dg/typebound_proc_20.f90
    trunk/gcc/testsuite/gfortran.dg/typebound_proc_21.f90
Modified:
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/gfortran.h
    trunk/gcc/fortran/trans-expr.c
    trunk/gcc/fortran/trans-types.c
    trunk/gcc/fortran/trans.h
Comment 49 Michael Matz 2011-02-18 19:55:00 UTC
Fixed.
Comment 50 Joost VandeVondele 2011-04-05 15:05:52 UTC
The original problem in comment #0 fails (i.e. the build of CP2K) with trunk 4.7, at what I believe is essentially the same issue. Unfortunately the smaller testcase (comment #1) doesn't fail anymore.

In file included from :363:0:
/data03/vondele/cp2k_gcc/cp2k/makefiles/../src/qs_linres_current.F: In function ‘calculate_jrho_resp’:
/data03/vondele/cp2k_gcc/cp2k/makefiles/../src/qs_linres_current.F:612:0: error: non-trivial conversion at assignment
struct array3_real(kind=8)
struct array3_real(kind=8)
# .MEM_3879 = VDEF <.MEM_3388>
my_gauge = D.66078_1423->r;

/data03/vondele/cp2k_gcc/cp2k/makefiles/../src/qs_linres_current.F:612:0: internal compiler error: verify_gimple failed
Please submit a full bug report,
with preprocessed source if appropriate.
See <http://gcc.gnu.org/bugs.html> for instructions.
make[3]: *** [/dev/shm/vondele/cciVnmd6.ltrans0.ltrans.o] Error 1
make[3]: Target `all' not remade because of errors.
lto-wrapper: make returned 2 exit status
/data03/vondele/gnu/binutils-2.21/install/bin/ld.gold: fatal error: lto-wrapper failed
collect2: ld returned 1 exit status
Comment 51 Tobias Burnus 2011-04-11 09:09:37 UTC
(In reply to comment #50)
> The original problem in comment #0 fails (i.e. the build of CP2K) with trunk
> 4.7, at what I believe is essentially the same issue.
> Unfortunately the smaller testcase (comment #1) doesn't fail anymore.

> /data03/vondele/cp2k_gcc/cp2k/makefiles/../src/qs_linres_current.F:
> In function ‘calculate_jrho_resp’:
> /data03/vondele/cp2k_gcc/cp2k/makefiles/../src/qs_linres_current.F:612:0:
> error: non-trivial conversion at assignment

Could you try to reduce the test case?
Comment 52 Joost VandeVondele 2011-04-12 05:50:45 UTC
(In reply to comment #51)
> (In reply to comment #50)
> > The original problem in comment #0 fails (i.e. the build of CP2K) with trunk
> Could you try to reduce the test case?

I will give it a try later this week, but this kind of LTO ICEs are difficult to reduce (starting from ~1MLOC), since it can basically not be done automatically (and the obvious testcase doesn't seem to fail anymore).
Comment 53 Joost VandeVondele 2011-04-13 18:47:24 UTC
reduced testcase for 4.7

MODULE M1
  INTEGER, PARAMETER :: dp=8
  TYPE realspace_grid_type
     REAL(KIND=dp), DIMENSION ( :, :, : ), ALLOCATABLE :: r
  END TYPE realspace_grid_type
  TYPE realspace_grid_p_type
     TYPE(realspace_grid_type), POINTER :: rs_grid
  END TYPE realspace_grid_p_type
  TYPE realspaces_grid_p_type
     TYPE(realspace_grid_p_type), DIMENSION(:), POINTER :: rs
  END TYPE realspaces_grid_p_type
END MODULE

MODULE M2
 USE M1
CONTAINS
 SUBROUTINE S1()
  INTEGER :: i,j
  TYPE(realspaces_grid_p_type), DIMENSION(:), POINTER :: rs_gauge
  REAL(dp), DIMENSION(:, :, :), POINTER    :: y
  y=>rs_gauge(i)%rs(j)%rs_grid%r
 END SUBROUTINE
END MODULE

USE M2
  CALL S1()
END

> gfortran -O0  -flto  test.f90
In file included from :0:0:
test.f90: In function ‘s1’:
test.f90:17:0: error: non-trivial conversion at assignment
struct array3_real(kind=8)
struct array3_real(kind=8)
y = D.2087_27->r;

test.f90:17:0: internal compiler error: verify_gimple failed
Comment 54 Tobias Burnus 2011-04-26 14:07:03 UTC
(In reply to comment #53)
> reduced testcase for 4.7

Does not fail here - can you still reproduce it? (It might have been fixed by the patch for PR 48588. If it still occurs, I will try harder.)
Comment 55 Joost VandeVondele 2011-04-26 18:17:52 UTC
(In reply to comment #54)
> (In reply to comment #53)
> > reduced testcase for 4.7
> 
> Does not fail here - can you still reproduce it? (It might have been fixed by
> the patch for PR 48588. If it still occurs, I will try harder.)

still fails for me with latest trunk (but notice the testcase only fails to compile at -O0 -flto). For completeness, the full -v output for me, might be gold or so related:

gfortran -v -O0 -flto t.f90 
Driving: gfortran -v -O0 -flto t.f90 -l gfortran -l m -shared-libgcc
Using built-in specs.
COLLECT_GCC=gfortran
COLLECT_LTO_WRAPPER=/data03/vondele/gnu/gcc_trunk/install/libexec/gcc/x86_64-unknown-linux-gnu/4.7.0/lto-wrapper
Target: x86_64-unknown-linux-gnu
Configured with: ../gcc/configure --prefix=/data03/vondele/gnu/gcc_trunk/install --enable-languages=c,c++,fortran --disable-multilib --enable-plugins --enable-cloog-backend=isl --with-ppl=/data03/vondele/gnu/ppl-0.11/install --with-cloog=/data03/vondele/gnu/cloog-0.16.1/install/ --with-libelf=/data03/vondele/gnu/libelf-0.8.13/install --with-plugin-ld=ld.gold
Thread model: posix
gcc version 4.7.0 20110426 (experimental) [trunk revision 172980] (GCC) 
COLLECT_GCC_OPTIONS='-v' '-O0' '-flto' '-shared-libgcc' '-mtune=generic' '-march=x86-64'
 /data03/vondele/gnu/gcc_trunk/install/libexec/gcc/x86_64-unknown-linux-gnu/4.7.0/f951 t.f90 -quiet -dumpbase t.f90 -mtune=generic -march=x86-64 -auxbase t -O0 -version -flto -fintrinsic-modules-path /data03/vondele/gnu/gcc_trunk/install/lib/gcc/x86_64-unknown-linux-gnu/4.7.0/finclude -o /dev/shm/vondele/cc5Lcxwt.s
GNU Fortran (GCC) version 4.7.0 20110426 (experimental) [trunk revision 172980] (x86_64-unknown-linux-gnu)
        compiled by GNU C version 4.7.0 20110426 (experimental) [trunk revision 172980], GMP version 4.3.2, MPFR version 2.4.2, MPC version 0.8.2
GGC heuristics: --param ggc-min-expand=30 --param ggc-min-heapsize=4096
GNU Fortran (GCC) version 4.7.0 20110426 (experimental) [trunk revision 172980] (x86_64-unknown-linux-gnu)
        compiled by GNU C version 4.7.0 20110426 (experimental) [trunk revision 172980], GMP version 4.3.2, MPFR version 2.4.2, MPC version 0.8.2
GGC heuristics: --param ggc-min-expand=30 --param ggc-min-heapsize=4096
COLLECT_GCC_OPTIONS='-v' '-O0' '-flto' '-shared-libgcc' '-mtune=generic' '-march=x86-64'
 as --64 -o /dev/shm/vondele/ccHOHV4Q.o /dev/shm/vondele/cc5Lcxwt.s
Reading specs from /data03/vondele/gnu/gcc_trunk/install/lib/gcc/x86_64-unknown-linux-gnu/4.7.0/../../../../lib64/libgfortran.spec
rename spec lib to liborig
COLLECT_GCC_OPTIONS='-v' '-O0' '-flto' '-shared-libgcc' '-mtune=generic' '-march=x86-64'
COMPILER_PATH=/data03/vondele/gnu/gcc_trunk/install/libexec/gcc/x86_64-unknown-linux-gnu/4.7.0/:/data03/vondele/gnu/gcc_trunk/install/libexec/gcc/x86_64-unknown-linux-gnu/4.7.0/:/data03/vondele/gnu/gcc_trunk/install/libexec/gcc/x86_64-unknown-linux-gnu/:/data03/vondele/gnu/gcc_trunk/install/lib/gcc/x86_64-unknown-linux-gnu/4.7.0/:/data03/vondele/gnu/gcc_trunk/install/lib/gcc/x86_64-unknown-linux-gnu/
LIBRARY_PATH=/data03/vondele/gnu/gcc_trunk/install/lib/gcc/x86_64-unknown-linux-gnu/4.7.0/:/data03/vondele/gnu/gcc_trunk/install/lib/gcc/x86_64-unknown-linux-gnu/4.7.0/../../../../lib64/:/lib/../lib64/:/usr/lib/../lib64/:/data03/vondele/gnu/gcc_trunk/install/lib/gcc/x86_64-unknown-linux-gnu/4.7.0/../../../:/lib/:/usr/lib/
COLLECT_GCC_OPTIONS='-v' '-O0' '-flto' '-shared-libgcc' '-mtune=generic' '-march=x86-64'
 /data03/vondele/gnu/gcc_trunk/install/libexec/gcc/x86_64-unknown-linux-gnu/4.7.0/collect2 -flto --eh-frame-hdr -m elf_x86_64 -dynamic-linker /lib64/ld-linux-x86-64.so.2 /usr/lib/../lib64/crt1.o /usr/lib/../lib64/crti.o /data03/vondele/gnu/gcc_trunk/install/lib/gcc/x86_64-unknown-linux-gnu/4.7.0/crtbegin.o -L/data03/vondele/gnu/gcc_trunk/install/lib/gcc/x86_64-unknown-linux-gnu/4.7.0 -L/data03/vondele/gnu/gcc_trunk/install/lib/gcc/x86_64-unknown-linux-gnu/4.7.0/../../../../lib64 -L/lib/../lib64 -L/usr/lib/../lib64 -L/data03/vondele/gnu/gcc_trunk/install/lib/gcc/x86_64-unknown-linux-gnu/4.7.0/../../.. /dev/shm/vondele/ccHOHV4Q.o -lgfortran -lm -lgcc_s -lgcc -lquadmath -lm -lgcc_s -lgcc -lc -lgcc_s -lgcc /data03/vondele/gnu/gcc_trunk/install/lib/gcc/x86_64-unknown-linux-gnu/4.7.0/crtend.o /usr/lib/../lib64/crtn.o
 gfortran @/dev/shm/vondele/ccrOSrYZ.args
Using built-in specs.
COLLECT_GCC=gfortran
COLLECT_LTO_WRAPPER=/data03/vondele/gnu/gcc_trunk/install/libexec/gcc/x86_64-unknown-linux-gnu/4.7.0/lto-wrapper
Target: x86_64-unknown-linux-gnu
Configured with: ../gcc/configure --prefix=/data03/vondele/gnu/gcc_trunk/install --enable-languages=c,c++,fortran --disable-multilib --enable-plugins --enable-cloog-backend=isl --with-ppl=/data03/vondele/gnu/ppl-0.11/install --with-cloog=/data03/vondele/gnu/cloog-0.16.1/install/ --with-libelf=/data03/vondele/gnu/libelf-0.8.13/install --with-plugin-ld=ld.gold
Thread model: posix
gcc version 4.7.0 20110426 (experimental) [trunk revision 172980] (GCC) 
COLLECT_GCC_OPTIONS='-c' '-v' '-O0' '-shared-libgcc' '-mtune=generic' '-march=x86-64' '-fltrans-output-list=/dev/shm/vondele/ccU5I08B.ltrans.out' '-fwpa'
 /data03/vondele/gnu/gcc_trunk/install/libexec/gcc/x86_64-unknown-linux-gnu/4.7.0/lto1 -quiet -dumpbase ccHOHV4Q.o -mtune=generic -march=x86-64 -auxbase ccHOHV4Q -O0 -version -fltrans-output-list=/dev/shm/vondele/ccU5I08B.ltrans.out -fwpa @/dev/shm/vondele/ccFk202B
GNU GIMPLE (GCC) version 4.7.0 20110426 (experimental) [trunk revision 172980] (x86_64-unknown-linux-gnu)
        compiled by GNU C version 4.7.0 20110426 (experimental) [trunk revision 172980], GMP version 4.3.2, MPFR version 2.4.2, MPC version 0.8.2
GGC heuristics: --param ggc-min-expand=30 --param ggc-min-heapsize=4096
GNU GIMPLE (GCC) version 4.7.0 20110426 (experimental) [trunk revision 172980] (x86_64-unknown-linux-gnu)
        compiled by GNU C version 4.7.0 20110426 (experimental) [trunk revision 172980], GMP version 4.3.2, MPFR version 2.4.2, MPC version 0.8.2
GGC heuristics: --param ggc-min-expand=30 --param ggc-min-heapsize=4096
COMPILER_PATH=/data03/vondele/gnu/gcc_trunk/install/libexec/gcc/x86_64-unknown-linux-gnu/4.7.0/:/data03/vondele/gnu/gcc_trunk/install/libexec/gcc/x86_64-unknown-linux-gnu/4.7.0/:/data03/vondele/gnu/gcc_trunk/install/libexec/gcc/x86_64-unknown-linux-gnu/:/data03/vondele/gnu/gcc_trunk/install/lib/gcc/x86_64-unknown-linux-gnu/4.7.0/:/data03/vondele/gnu/gcc_trunk/install/lib/gcc/x86_64-unknown-linux-gnu/:/data03/vondele/gnu/gcc_trunk/install/libexec/gcc/x86_64-unknown-linux-gnu/4.7.0/:/data03/vondele/gnu/gcc_trunk/install/libexec/gcc/x86_64-unknown-linux-gnu/4.7.0/:/data03/vondele/gnu/gcc_trunk/install/libexec/gcc/x86_64-unknown-linux-gnu/:/data03/vondele/gnu/gcc_trunk/install/lib/gcc/x86_64-unknown-linux-gnu/4.7.0/:/data03/vondele/gnu/gcc_trunk/install/lib/gcc/x86_64-unknown-linux-gnu/
LIBRARY_PATH=/data03/vondele/gnu/gcc_trunk/install/lib/gcc/x86_64-unknown-linux-gnu/4.7.0/../../../../lib64/../lib64/:/lib/../lib64/../lib64/:/usr/lib/../lib64/../lib64/:/data03/vondele/gnu/gcc_trunk/install/lib/gcc/x86_64-unknown-linux-gnu/4.7.0/../../../../lib64/:/lib/../lib64/:/usr/lib/../lib64/:/data03/vondele/gnu/gcc_trunk/install/lib/gcc/x86_64-unknown-linux-gnu/4.7.0/:/data03/vondele/gnu/gcc_trunk/install/lib/gcc/x86_64-unknown-linux-gnu/4.7.0/../../../../lib64/:/lib/../lib64/:/usr/lib/../lib64/:/data03/vondele/gnu/gcc_trunk/install/lib/gcc/x86_64-unknown-linux-gnu/4.7.0/:/data03/vondele/gnu/gcc_trunk/install/lib/gcc/x86_64-unknown-linux-gnu/4.7.0/../../../../lib64/:/lib/../lib64/:/usr/lib/../lib64/:/data03/vondele/gnu/gcc_trunk/install/lib/gcc/x86_64-unknown-linux-gnu/4.7.0/../../../:/lib/:/usr/lib/:/data03/vondele/gnu/gcc_trunk/install/lib/gcc/x86_64-unknown-linux-gnu/4.7.0/../../../:/lib/:/usr/lib/
COLLECT_GCC_OPTIONS='-c' '-v' '-O0' '-shared-libgcc' '-mtune=generic' '-march=x86-64' '-fltrans-output-list=/dev/shm/vondele/ccU5I08B.ltrans.out' '-fwpa'
 gfortran @/dev/shm/vondele/ccbrXBHL.args
Using built-in specs.
COLLECT_GCC=gfortran
COLLECT_LTO_WRAPPER=/data03/vondele/gnu/gcc_trunk/install/libexec/gcc/x86_64-unknown-linux-gnu/4.7.0/lto-wrapper
Target: x86_64-unknown-linux-gnu
Configured with: ../gcc/configure --prefix=/data03/vondele/gnu/gcc_trunk/install --enable-languages=c,c++,fortran --disable-multilib --enable-plugins --enable-cloog-backend=isl --with-ppl=/data03/vondele/gnu/ppl-0.11/install --with-cloog=/data03/vondele/gnu/cloog-0.16.1/install/ --with-libelf=/data03/vondele/gnu/libelf-0.8.13/install --with-plugin-ld=ld.gold
Thread model: posix
gcc version 4.7.0 20110426 (experimental) [trunk revision 172980] (GCC) 
COLLECT_GCC_OPTIONS='-c' '-v' '-O0' '-shared-libgcc' '-mtune=generic' '-march=x86-64' '-fltrans' '-o' '/dev/shm/vondele/ccU5I08B.ltrans0.ltrans.o'
 /data03/vondele/gnu/gcc_trunk/install/libexec/gcc/x86_64-unknown-linux-gnu/4.7.0/lto1 -quiet -dumpbase ccU5I08B.ltrans0.o -mtune=generic -march=x86-64 -auxbase-strip /dev/shm/vondele/ccU5I08B.ltrans0.ltrans.o -O0 -version -fltrans @/dev/shm/vondele/ccZW67eE -o /dev/shm/vondele/ccktUf91.s
GNU GIMPLE (GCC) version 4.7.0 20110426 (experimental) [trunk revision 172980] (x86_64-unknown-linux-gnu)
        compiled by GNU C version 4.7.0 20110426 (experimental) [trunk revision 172980], GMP version 4.3.2, MPFR version 2.4.2, MPC version 0.8.2
GGC heuristics: --param ggc-min-expand=30 --param ggc-min-heapsize=4096
GNU GIMPLE (GCC) version 4.7.0 20110426 (experimental) [trunk revision 172980] (x86_64-unknown-linux-gnu)
        compiled by GNU C version 4.7.0 20110426 (experimental) [trunk revision 172980], GMP version 4.3.2, MPFR version 2.4.2, MPC version 0.8.2
GGC heuristics: --param ggc-min-expand=30 --param ggc-min-heapsize=4096
In file included from :0:0:
t.f90: In function ‘s1’:
t.f90:17:0: error: non-trivial conversion at assignment
struct array3_real(kind=8)
struct array3_real(kind=8)
y = D.2088_27->r;

t.f90:17:0: internal compiler error: verify_gimple failed
Please submit a full bug report,
with preprocessed source if appropriate.
See <http://gcc.gnu.org/bugs.html> for instructions.
lto-wrapper: gfortran returned 1 exit status
collect2: lto-wrapper returned 1 exit status
Comment 56 Joost VandeVondele 2011-04-26 18:19:29 UTC
(In reply to comment #54)
> (In reply to comment #53)
> > reduced testcase for 4.7
> 
> Does not fail here - can you still reproduce it? (It might have been fixed by
> the patch for PR 48588. If it still occurs, I will try harder.)

an as a PS, --enable-checking=release also hides the bug.
Comment 57 Thomas Koenig 2011-04-26 19:37:32 UTC
(In reply to comment #54)
> (In reply to comment #53)
> > reduced testcase for 4.7
> 
> Does not fail here - can you still reproduce it? (It might have been fixed by
> the patch for PR 48588. If it still occurs, I will try harder.)

Fails for me here too, at -O0.

Looking at the site of the error message, we get (note the "public unsigned DI" vs. "unsigned restrict DI":

3722          error ("non-trivial conversion at assignment");                         
(gdb) p debug_tree(lhs_type)                                                          
 <record_type 0x7ffff70dae70 array3_real(kind=8) BLK                                  
    size <integer_cst 0x7ffff70d1420 type <integer_type 0x7ffff7ee50a8 bit_size_type> constant 768>
    unit size <integer_cst 0x7ffff70d1460 type <integer_type 0x7ffff7ee5000> constant 96>          
    align 64 symtab 0 alias set -1 canonical type 0x7ffff70daf18                                   
    fields <field_decl 0x7ffff70df4c0 data                                                         
        type <pointer_type 0x7ffff7ee5dc8 type <void_type 0x7ffff7ee5d20 void>                     
            public unsigned DI                                                                     
            size <integer_cst 0x7ffff7eea1e0 constant 64>                                          
            unit size <integer_cst 0x7ffff7eea200 constant 8>                                      
            align 64 symtab 0 alias set -1 canonical type 0x7ffff7ee5dc8                           
            pointer_to_this <pointer_type 0x7ffff7ef7e70>>                                         
        unsigned DI file test.f90 line 17 col 0 size <integer_cst 0x7ffff7eea1e0 64> unit size <integer_cst 0x7ffff7eea200 8>                                                                                       
        align 64 offset_align 128                                                                         
        offset <integer_cst 0x7ffff7ed5f00 constant 0>                                                    
        bit offset <integer_cst 0x7ffff7eea420 constant 0> context <record_type 0x7ffff70daf18 array_descriptor3>                                                                                                   
        chain <field_decl 0x7ffff70df558 offset type <integer_type 0x7ffff7ee55e8 long int>               
            DI file test.f90 line 17 col 0 size <integer_cst 0x7ffff7eea1e0 64> unit size <integer_cst 0x7ffff7eea200 8>                                                                                            
            align 64 offset_align 128 offset <integer_cst 0x7ffff7ed5f00 0> bit offset <integer_cst 0x7ffff7eea1e0 64> context <record_type 0x7ffff70daf18 array_descriptor3> chain <field_decl 0x7ffff70df5f0 dtype>>>>                                                                                                      
$1 = void                                                                                                 
(gdb) p debug_tree(rhs1_type)                                                                             
 <record_type 0x7ffff70da7e0 array3_real(kind=8) BLK                                                      
    size <integer_cst 0x7ffff70d1420 type <integer_type 0x7ffff7ee50a8 bit_size_type> constant 768>       
    unit size <integer_cst 0x7ffff70d1460 type <integer_type 0x7ffff7ee5000> constant 96>                 
    align 64 symtab 0 alias set -1 canonical type 0x7ffff70da930                                          
    fields <field_decl 0x7ffff7ededa8 data                                                                
        type <pointer_type 0x7ffff70da888 type <void_type 0x7ffff7ee5d20 void>                            
            unsigned restrict DI                                                                          
            size <integer_cst 0x7ffff7eea1e0 constant 64>                                                 
            unit size <integer_cst 0x7ffff7eea200 constant 8>                                             
            align 64 symtab 0 alias set -1 canonical type 0x7ffff70da888>                                 
        unsigned DI file test.f90 line 17 col 0 size <integer_cst 0x7ffff7eea1e0 64> unit size <integer_cst 0x7ffff7eea200 8>                                                                                       
        align 64 offset_align 128                                                                         
        offset <integer_cst 0x7ffff7ed5f00 constant 0>                                                    
        bit offset <integer_cst 0x7ffff7eea420 constant 0> context <record_type 0x7ffff70da930 array_descriptor3>                                                                                                   
        chain <field_decl 0x7ffff7edee40 offset type <integer_type 0x7ffff7ee55e8 long int>               
            DI file test.f90 line 17 col 0 size <integer_cst 0x7ffff7eea1e0 64> unit size <integer_cst 0x7ffff7eea200 8>                                                                                            
            align 64 offset_align 128 offset <integer_cst 0x7ffff7ed5f00 0> bit offset <integer_cst 0x7ffff7eea1e0 64> context <record_type 0x7ffff70da930 array_descriptor3> chain <field_decl 0x7ffff7edeed8 dtype>>>>                                                                                                      
$2 = void                                                                                                 
(gdb)                                                                                                     
 <record_type 0x7ffff70da7e0 array3_real(kind=8) BLK                                                      
    size <integer_cst 0x7ffff70d1420 type <integer_type 0x7ffff7ee50a8 bit_size_type> constant 768>       
    unit size <integer_cst 0x7ffff70d1460 type <integer_type 0x7ffff7ee5000> constant 96>                 
    align 64 symtab 0 alias set -1 canonical type 0x7ffff70da930                                          
    fields <field_decl 0x7ffff7ededa8 data                                                                
        type <pointer_type 0x7ffff70da888 type <void_type 0x7ffff7ee5d20 void>                            
            unsigned restrict DI                                                                          
            size <integer_cst 0x7ffff7eea1e0 constant 64>                                                 
            unit size <integer_cst 0x7ffff7eea200 constant 8>                                             
            align 64 symtab 0 alias set -1 canonical type 0x7ffff70da888>                                 
        unsigned DI file test.f90 line 17 col 0 size <integer_cst 0x7ffff7eea1e0 64> unit size <integer_cst 0x7ffff7eea200 8>                                                                                       
        align 64 offset_align 128                                                                         
        offset <integer_cst 0x7ffff7ed5f00 constant 0>                                                    
        bit offset <integer_cst 0x7ffff7eea420 constant 0> context <record_type 0x7ffff70da930 array_descriptor3>                                                                                                   
        chain <field_decl 0x7ffff7edee40 offset type <integer_type 0x7ffff7ee55e8 long int>               
            DI file test.f90 line 17 col 0 size <integer_cst 0x7ffff7eea1e0 64> unit size <integer_cst 0x7ffff7eea200 8>                                                                                            
            align 64 offset_align 128 offset <integer_cst 0x7ffff7ed5f00 0> bit offset <integer_cst 0x7ffff7eea1e0 64> context <record_type 0x7ffff70da930 array_descriptor3> chain <field_decl 0x7ffff7edeed8 dtype>>>>                                                                                                      
$3 = void
Comment 58 Jakub Jelinek 2011-06-27 12:32:56 UTC
GCC 4.6.1 is being released.
Comment 59 Tobias Burnus 2011-07-26 14:27:43 UTC
(In reply to comment #53)
> reduced testcase for 4.7
>   y=>rs_gauge(i)%rs(j)%rs_grid%r

Here, "%r->data" is marked as restricted as "r" is allocatable. However, all other references are a pointer.

The derived type is obtained via a nested calls to gfc_get_derived_type. For "r" one calls gfc_get_array_type_bounds - seemingly with "restricted = 1".

If one now declares "rs_gauge", one calls gfc_nonrestricted_type which removes the "restrict" qualifier from the type (i.e. "rs_gauge") and from the "rs_gauge->rs" but does not go higher up the chain.

Thus, one somehow needs to walk the components and remove the "restrict" there ...


With the following patch, one does not get any ICE anymore, but I am neither sure whether it is sufficient, nor whether it removes restrict qualifiers where it shouldn't, nor whether it is correct at all.


--- a/gcc/fortran/trans-types.c
+++ b/gcc/fortran/trans-types.c
@@ -2421,6 +2421,9 @@ gfc_get_derived_type (gfc_symbol * derived)
               && !c->attr.proc_pointer)
        field_type = build_pointer_type (field_type);

+      if (c->attr.pointer)
+       field_type = gfc_nonrestricted_type (field_type);
+
       /* vtype fields can point to different types to the base type.  */
       if (c->ts.type == BT_DERIVED && c->ts.u.derived->attr.vtype)
          field_type = build_pointer_type_for_mode (TREE_TYPE (field_type),
Comment 60 Tobias Burnus 2011-07-27 22:33:03 UTC
Author: burnus
Date: Wed Jul 27 22:33:00 2011
New Revision: 176852

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=176852
Log:
2011-07-27  Tobias Burnus  <burnus@net-b.de>

        PR fortran/45586
        * trans-types.c (gfc_get_derived_type): Ensure that pointer
        component types are marked as nonrestricted.

2011-07-27  Tobias Burnus  <burnus@net-b.de>

        PR fortran/45586
        * gfortran.dg/lto/pr45586-2_0.f90: New.


Added:
    trunk/gcc/testsuite/gfortran.dg/lto/pr45586-2_0.f90
Modified:
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/trans-types.c
    trunk/gcc/testsuite/ChangeLog
Comment 61 Tobias Burnus 2011-07-28 05:40:28 UTC
Author: burnus
Date: Thu Jul 28 05:40:21 2011
New Revision: 176858

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=176858
Log:
2011-07-28  Tobias Burnus  <burnus@net-b.de>

        PR fortran/45586
        * trans-types.c (gfc_get_derived_type): Ensure that pointer
        component types are marked as nonrestricted.

2011-07-28  Tobias Burnus  <burnus@net-b.de>

        PR fortran/45586
        * gfortran.dg/lto/pr45586-2_0.f90: New.


Added:
    branches/gcc-4_6-branch/gcc/testsuite/gfortran.dg/lto/pr45586-2_0.f90
Modified:
    branches/gcc-4_6-branch/gcc/fortran/ChangeLog
    branches/gcc-4_6-branch/gcc/fortran/trans-types.c
    branches/gcc-4_6-branch/gcc/testsuite/ChangeLog
Comment 62 Tobias Burnus 2011-07-28 05:42:01 UTC
FIXED the issue of comment 53 on the 4.7 trunk and on the 4.6 branch. Thanks for the report!
Comment 63 Richard Biener 2012-02-14 13:01:09 UTC
With a (seemingly) unrelated patch (attached to PR52097) I'm back on ICEing
for the gfortran.dg/lto/pr45586*.f90 testcases ...

Even before the adjusted type merging we have (at compile-time)

Breakpoint 5, output_gimple_stmt (ob=0x1c6c440, stmt=0x7ffff5b51730)
    at /space/rguenther/src/svn/trunk/gcc/gimple-streamer-out.c:138
138               stream_write_tree (ob, op, true);
y = D.1925_27->r;

(gdb) call debug_tree (gimple_assign_lhs (stmt)->typed.type)
 <record_type 0x7ffff5b4d2a0 array3_real(kind=8) type_1 BLK
    size <integer_cst 0x7ffff5b3e700 type <integer_type 0x7ffff5a2e0a8 bitsizetype> constant 768>
    unit size <integer_cst 0x7ffff5b1fc60 type <integer_type 0x7ffff5a2e000 sizetype> constant 96>
    align 64 symtab 0 alias set -1 canonical type 0x7ffff5b4a0a8
    fields <field_decl 0x7ffff5b3d7b8 data
        type <pointer_type 0x7ffff5a2ec78 type <void_type 0x7ffff5a2ebd0 void>
            public unsigned DI
            size <integer_cst 0x7ffff5a1dec0 constant 64>
            unit size <integer_cst 0x7ffff5a1dee0 constant 8>
            align 64 symtab 0 alias set -1 canonical type 0x7ffff5a2ec78
            pointer_to_this <pointer_type 0x7ffff5a41f18>>

(gdb) call debug_tree (gimple_assign_rhs1 (stmt)->typed.type)
 <record_type 0x7ffff5b4a738 array3_real(kind=8) type_1 BLK
    size <integer_cst 0x7ffff5b3e700 type <integer_type 0x7ffff5a2e0a8 bitsizetype> constant 768>
    unit size <integer_cst 0x7ffff5b1fc60 type <integer_type 0x7ffff5a2e000 sizetype> constant 96>
    align 64 symtab 0 alias set -1 canonical type 0x7ffff5b4a0a8
    fields <field_decl 0x7ffff5b3dab0 data
        type <pointer_type 0x7ffff5a2ec78 type <void_type 0x7ffff5a2ebd0 void>
            public unsigned DI
            size <integer_cst 0x7ffff5a1dec0 constant 64>
            unit size <integer_cst 0x7ffff5a1dee0 constant 8>
            align 64 symtab 0 alias set -1 canonical type 0x7ffff5a2ec78
            pointer_to_this <pointer_type 0x7ffff5a41f18>>

_BUT_(!) its main-variant is

(gdb) call debug_tree (gimple_assign_rhs1 (stmt)->typed.type->type_common.main_variant)
 <record_type 0x7ffff5b4a000 array3_real(kind=8) type_1 BLK
    size <integer_cst 0x7ffff5b3e700 type <integer_type 0x7ffff5a2e0a8 bitsizetype> constant 768>
    unit size <integer_cst 0x7ffff5b1fc60 type <integer_type 0x7ffff5a2e000 sizetype> constant 96>
    align 64 symtab 0 alias set -1 canonical type 0x7ffff5b4a0a8
    fields <field_decl 0x7ffff5b3d390 data
        type <pointer_type 0x7ffff5a41e70 type <void_type 0x7ffff5a2ebd0 void>
            unsigned restrict DI
            size <integer_cst 0x7ffff5a1dec0 constant 64>
            unit size <integer_cst 0x7ffff5a1dee0 constant 8>
            align 64 symtab 0 alias set -1 canonical type 0x7ffff5a41e70>

thus has a restrict-qualified data field.  That's bogus as TYPE_FIELDS
is supposed to be shared amongst variant types.
Comment 64 Richard Biener 2012-02-14 13:20:37 UTC
Index: gcc/fortran/trans-types.c
===================================================================
--- gcc/fortran/trans-types.c   (revision 184203)
+++ gcc/fortran/trans-types.c   (working copy)
@@ -2042,7 +2042,8 @@ gfc_nonrestricted_type (tree t)
              }
          if (!field)
            break;
-         ret = build_variant_type_copy (t);
+         ret = build_distinct_type_copy (t);
+         TYPE_CANONICAL (ret) = TYPE_CANONICAL (t);
          TYPE_FIELDS (ret) = NULL_TREE;
 
          /* Here we make sure that as soon as we know we have to copy

works here, but I suspect it would not fix the Fortran -flto ICEs in
PR51765 (which happen on pristine trunk, much to the same issue I suppose).
Comment 65 Richard Biener 2012-03-07 09:47:16 UTC
*** Bug 52516 has been marked as a duplicate of this bug. ***
Comment 66 Richard Biener 2012-03-07 09:48:12 UTC
Now exposed again.
Comment 67 Dominique d'Humieres 2012-03-12 10:15:39 UTC
The patch in comment #64 fixes the failures reported in pr52516 but introduces many regressions:

		=== gfortran Summary for unix/-m64 ===

# of expected passes		41076
# of unexpected failures	92
# of expected failures		56
# of unresolved testcases	12
# of unsupported tests		71

		=== gfortran Summary ===

# of expected passes		81865
# of unexpected failures	184
# of expected failures		112
# of unresolved testcases	24
# of unsupported tests		280

The failing tests are

FAIL: gfortran.dg/alloc_comp_assign_10.f90  -O0  (internal compiler error)
FAIL: gfortran.dg/alloc_comp_basics_2.f90  -O0  (internal compiler error)
FAIL: gfortran.dg/alloc_comp_initializer_1.f90  -O0  (internal compiler error)
FAIL: gfortran.dg/dependency_25.f90  -O0  (internal compiler error)
FAIL: gfortran.dg/dependency_37.f90  -O  (internal compiler error)
FAIL: gfortran.dg/typebound_proc_20.f90  -O  (internal compiler error)
FAIL: gfortran.dg/whole_file_31.f90  -O  (internal compiler error)
FAIL: gfortran.dg/lto/pr45586 f_lto_pr45586_0.o assemble, -O0 -flto -flto-partition=none  (internal compiler error)

and the internal compiler errors are

/opt/gcc/work/gcc/testsuite/gfortran.dg/alloc_comp_assign_10.f90: In function 'tao_program':
/opt/gcc/work/gcc/testsuite/gfortran.dg/alloc_comp_assign_10.f90:48:0: internal compiler error: in fold_convert_loc, at fold-const.c:2016

> ... but I suspect it would not fix the Fortran -flto ICEs in
> PR51765 (which happen on pristine trunk, much to the same issue I suppose).

confirmed AFACT:

		=== gfortran Summary for unix/-m64/-flto ===

# of expected passes		40973
# of unexpected failures	186
# of expected failures		56
# of unresolved testcases	12
# of unsupported tests		71

		=== gfortran Summary ===

# of expected passes		81659
# of unexpected failures	372
# of expected failures		112
# of unresolved testcases	24
# of unsupported tests		280
Comment 68 Richard Biener 2012-05-11 08:31:57 UTC
*** Bug 53318 has been marked as a duplicate of this bug. ***
Comment 69 Mikael Morin 2012-07-28 09:46:00 UTC
(In reply to comment #63)
> With a (seemingly) unrelated patch (attached to PR52097) I'm back on ICEing
> for the gfortran.dg/lto/pr45586*.f90 testcases ...
> 
> Even before the adjusted type merging we have (at compile-time)
> 
[...]
> 
> _BUT_(!) its main-variant is
> 
[...]
> 
> thus has a restrict-qualified data field.  That's bogus as TYPE_FIELDS
> is supposed to be shared amongst variant types.

So, is this a fortran front-end bug, a middle-end bug, or a lto bug ?
(Hint: PR 51765 is a marked as a lto bug ;-) )
Comment 70 rguenther@suse.de 2012-07-30 08:43:09 UTC
On Sat, 28 Jul 2012, mikael at gcc dot gnu.org wrote:

> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45586
> 
> --- Comment #69 from Mikael Morin <mikael at gcc dot gnu.org> 2012-07-28 09:46:00 UTC ---
> (In reply to comment #63)
> > With a (seemingly) unrelated patch (attached to PR52097) I'm back on ICEing
> > for the gfortran.dg/lto/pr45586*.f90 testcases ...
> > 
> > Even before the adjusted type merging we have (at compile-time)
> > 
> [...]
> > 
> > _BUT_(!) its main-variant is
> > 
> [...]
> > 
> > thus has a restrict-qualified data field.  That's bogus as TYPE_FIELDS
> > is supposed to be shared amongst variant types.
> 
> So, is this a fortran front-end bug, a middle-end bug, or a lto bug ?
> (Hint: PR 51765 is a marked as a lto bug ;-) )

It's a frontend bug.
Comment 71 Mikael Morin 2012-07-30 10:35:48 UTC
(In reply to comment #70)
> On Sat, 28 Jul 2012, mikael at gcc dot gnu.org wrote:
> > So, is this a fortran front-end bug, a middle-end bug, or a lto bug ?
> > (Hint: PR 51765 is a marked as a lto bug ;-) )
> 
> It's a frontend bug.

great :-(.


(In reply to comment #63)
> That's bogus as TYPE_FIELDS
> is supposed to be shared amongst variant types.

Then we'll have to revert Micha's recursive restrict work.
Is it possible for the front-end to specify alias sets by hand (I mean without relying on the middle-end computing them based on types etc)?
Comment 72 rguenther@suse.de 2012-07-30 11:04:39 UTC
On Mon, 30 Jul 2012, mikael at gcc dot gnu.org wrote:

> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45586
> 
> --- Comment #71 from Mikael Morin <mikael at gcc dot gnu.org> 2012-07-30 10:35:48 UTC ---
> (In reply to comment #70)
> > On Sat, 28 Jul 2012, mikael at gcc dot gnu.org wrote:
> > > So, is this a fortran front-end bug, a middle-end bug, or a lto bug ?
> > > (Hint: PR 51765 is a marked as a lto bug ;-) )
> > 
> > It's a frontend bug.
> 
> great :-(.
> 
> 
> (In reply to comment #63)
> > That's bogus as TYPE_FIELDS
> > is supposed to be shared amongst variant types.
> 
> Then we'll have to revert Micha's recursive restrict work.

I don't think so, it merely has to be fixed.

> Is it possible for the front-end to specify alias sets by hand (I mean without
> relying on the middle-end computing them based on types etc)?

Yes.  But that does not work with LTO, nor does it address the original
issue of supporting INTENT IN/OUT properly.
Comment 73 Mikael Morin 2012-07-30 12:29:33 UTC
(In reply to comment #72)
> > (In reply to comment #63)
> > > That's bogus as TYPE_FIELDS
> > > is supposed to be shared amongst variant types.
> > 
> > Then we'll have to revert Micha's recursive restrict work.
> 
> I don't think so, it merely has to be fixed.
How so? by making it non-recursive?
For variable to be type compatible for assignment, they shall be variants of the same type, and thus have the same TYPE_FIELDS.
If they shall have the same TYPE_FIELDS, they shall have the same components, the same components have the same types, so that one cannot be restrict qualified.

> 
> > Is it possible for the front-end to specify alias sets by hand (I mean without
> > relying on the middle-end computing them based on types etc)?
> 
> Yes.  But that does not work with LTO,
You mean calling the front-end's code does not work at LTO time or the alias sets are not saved/restored for LTO?

> nor does it address the original
> issue of supporting INTENT IN/OUT properly.
Ah? Isn't a restricted type variable (resp. component, etc) merely one that has its own alias set? So if it works with restrict, it works with alias sets?
Comment 74 rguenther@suse.de 2012-07-30 12:33:01 UTC
On Mon, 30 Jul 2012, mikael at gcc dot gnu.org wrote:

> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45586
> 
> --- Comment #73 from Mikael Morin <mikael at gcc dot gnu.org> 2012-07-30 12:29:33 UTC ---
> (In reply to comment #72)
> > > (In reply to comment #63)
> > > > That's bogus as TYPE_FIELDS
> > > > is supposed to be shared amongst variant types.
> > > 
> > > Then we'll have to revert Micha's recursive restrict work.
> > 
> > I don't think so, it merely has to be fixed.
> How so? by making it non-recursive?
> For variable to be type compatible for assignment, they shall be variants of
> the same type, and thus have the same TYPE_FIELDS.
> If they shall have the same TYPE_FIELDS, they shall have the same components,
> the same components have the same types, so that one cannot be restrict
> qualified.

The types should not be variant types ;)  (see my hack patch in
this or another bug)

> > > Is it possible for the front-end to specify alias sets by hand (I mean without
> > > relying on the middle-end computing them based on types etc)?
> > 
> > Yes.  But that does not work with LTO,
> You mean calling the front-end's code does not work at LTO time or the alias
> sets are not saved/restored for LTO?

LTO re-computes alias-sets based on types.

> > nor does it address the original
> > issue of supporting INTENT IN/OUT properly.
> Ah? Isn't a restricted type variable (resp. component, etc) merely one that has
> its own alias set? So if it works with restrict, it works with alias sets?

No, alias-sets and restrict are different beasts.  It's important to
have the data member restrict qualified for INTENT IN/OUT.
Comment 75 Mikael Morin 2012-08-01 12:22:03 UTC
Created attachment 27919 [details]
rough patch

(In reply to comment #74)
> > For variable to be type compatible for assignment, they shall be variants of
> > the same type, and thus have the same TYPE_FIELDS.
> > If they shall have the same TYPE_FIELDS, they shall have the same components,
> > the same components have the same types, so that one cannot be restrict
> > qualified.
> 
> The types should not be variant types ;)  (see my hack patch in
> this or another bug)
Ah OK, I thought it was just a hack.

> > > nor does it address the original
> > > issue of supporting INTENT IN/OUT properly.
> > Ah? Isn't a restricted type variable (resp. component, etc) merely one that has
> > its own alias set? So if it works with restrict, it works with alias sets?
> 
> No, alias-sets and restrict are different beasts.  It's important to
> have the data member restrict qualified for INTENT IN/OUT.
Ah.

I have attached a (very rough) patch which is already in the testsuite past the first failing cases reported by Dominique.
About your fix:
(In reply to comment #64)
> Index: gcc/fortran/trans-types.c
> ===================================================================
> --- gcc/fortran/trans-types.c   (revision 184203)
> +++ gcc/fortran/trans-types.c   (working copy)
> @@ -2042,7 +2042,8 @@ gfc_nonrestricted_type (tree t)
>               }
>           if (!field)
>             break;
> -         ret = build_variant_type_copy (t);
> +         ret = build_distinct_type_copy (t);
> +         TYPE_CANONICAL (ret) = TYPE_CANONICAL (t);
>           TYPE_FIELDS (ret) = NULL_TREE;
> 
>           /* Here we make sure that as soon as we know we have to copy
there is another call to build_variant_type_copy before about array types.
Should the same fix by applied or are variants OK there?
Comment 76 rguenther@suse.de 2012-08-01 12:28:10 UTC
On Wed, 1 Aug 2012, mikael at gcc dot gnu.org wrote:

> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45586
> 
> --- Comment #75 from Mikael Morin <mikael at gcc dot gnu.org> 2012-08-01 12:22:03 UTC ---
> Created attachment 27919 [details]
>   --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=27919
> rough patch
> 
> (In reply to comment #74)
> > > For variable to be type compatible for assignment, they shall be variants of
> > > the same type, and thus have the same TYPE_FIELDS.
> > > If they shall have the same TYPE_FIELDS, they shall have the same components,
> > > the same components have the same types, so that one cannot be restrict
> > > qualified.
> > 
> > The types should not be variant types ;)  (see my hack patch in
> > this or another bug)
> Ah OK, I thought it was just a hack.
> 
> > > > nor does it address the original
> > > > issue of supporting INTENT IN/OUT properly.
> > > Ah? Isn't a restricted type variable (resp. component, etc) merely one that has
> > > its own alias set? So if it works with restrict, it works with alias sets?
> > 
> > No, alias-sets and restrict are different beasts.  It's important to
> > have the data member restrict qualified for INTENT IN/OUT.
> Ah.
> 
> I have attached a (very rough) patch which is already in the testsuite past the
> first failing cases reported by Dominique.
> About your fix:
> (In reply to comment #64)
> > Index: gcc/fortran/trans-types.c
> > ===================================================================
> > --- gcc/fortran/trans-types.c   (revision 184203)
> > +++ gcc/fortran/trans-types.c   (working copy)
> > @@ -2042,7 +2042,8 @@ gfc_nonrestricted_type (tree t)
> >               }
> >           if (!field)
> >             break;
> > -         ret = build_variant_type_copy (t);
> > +         ret = build_distinct_type_copy (t);
> > +         TYPE_CANONICAL (ret) = TYPE_CANONICAL (t);
> >           TYPE_FIELDS (ret) = NULL_TREE;
> > 
> >           /* Here we make sure that as soon as we know we have to copy
> there is another call to build_variant_type_copy before about array types.
> Should the same fix by applied or are variants OK there?

You mean

      case ARRAY_TYPE:
        {
          tree elemtype = gfc_nonrestricted_type (TREE_TYPE (t));
          if (elemtype == TREE_TYPE (t))
            ret = t;
          else
            {
              ret = build_variant_type_copy (t);

?  Yes, that also should be build_distinct_type_copy.

Richard.
Comment 77 Mikael Morin 2012-08-01 12:37:45 UTC
(In reply to comment #75)
> Created attachment 27919 [details]
> rough patch
> 
About the patch:

The failures are mostly(all?) due to structure constructors.
Structure constructors use components' backend_decl which are of restricted types.
however, if the lhs is a target, one should use the non-restricted components.
I tried fixing the constructor using gfc_convert (see the patch).
However, it doesn't work with scalarization, as the constructor is evaluated
outside the loop into a variable (which gets the restricted type).

So, this patch adds a flag restricted to gfc_conv_structure.
the flag propagates to gfc_conv_expr, and gfc_conv_initializer, gfc_conv_initializer, etc.
To know whether we need to call gfc_conv_expr with the restricted flag set or not in the scalarizer, a new flag field is added to the scalarizer's gfc_ss_info structures to tell whether we want a non-restricted expression. For it to be set appropriately, a flag is propagated to the gfc_walk_* functions.
Comment 78 Mikael Morin 2012-08-01 15:01:59 UTC
(In reply to comment #76)
> You mean
> 
> [...]
> 
> ?  Yes, that also should be build_distinct_type_copy.
> 
Even without that, the patch regtests cleanly (including the pr45586 tests), apart for typebound_proc_20.f90. That one fail in the following line from the `calc' subroutine:

        this%y = this%find_y()  ! FAILS

the lhs is a target, and the rhs is NOT a target, so that the middle-end types are different. :-(
Comment 79 rguenther@suse.de 2012-08-01 15:05:22 UTC
On Wed, 1 Aug 2012, mikael at gcc dot gnu.org wrote:

> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45586
> 
> --- Comment #78 from Mikael Morin <mikael at gcc dot gnu.org> 2012-08-01 15:01:59 UTC ---
> (In reply to comment #76)
> > You mean
> > 
> > [...]
> > 
> > ?  Yes, that also should be build_distinct_type_copy.
> > 
> Even without that, the patch regtests cleanly (including the pr45586 tests),
> apart for typebound_proc_20.f90. That one fail in the following line from the
> `calc' subroutine:
> 
>         this%y = this%find_y()  ! FAILS
> 
> the lhs is a target, and the rhs is NOT a target, so that the middle-end types
> are different. :-(

But how can this be a valid fortran program?  You assign something
that is not aliased to something that suddenly makes it aliased?
If that's valid then you can make the middle-end happy by wrapping
the RHS inside a VIEW_CONVERT_EXPR with the LHS type.
Comment 80 Tobias Burnus 2012-08-01 16:22:52 UTC
(In reply to comment #79)
> >         this%y = this%find_y()  ! FAILS
> > 
> > the lhs is a target, and the rhs is NOT a target, so that the middle-end types
> > are different. :-(
> 
> But how can this be a valid fortran program?  You assign something
> that is not aliased to something that suddenly makes it aliased?
> If that's valid then you can make the middle-end happy by wrapping
> the RHS inside a VIEW_CONVERT_EXPR with the LHS type.

I have not closely looked at the dump, however,
  this%y = this%find_y()
means that one assigns component-wise the values from the RHS to the LHS; if there are pointer components, the pointer address is assigned; if there are allocatable components, those are - if needed - first (re)allocated and then (element-wise) assigned.

Thus, one only assigns the values and no pointers - and, hence, the RHS can be a nontarget while the LHS can be a target.

In this example, "this%y" is a derived type with an allocatable-array component. I think the current algorithm is something like:

  *dst = *src;
  if (src->data)
    {
      (re)allocate dst->data
      memcpy (dst->data, src->data);
    }

Thus, one first transfers the pointer address [besides the array bounds], but if "data" is not NULL, the data is copied. Thus, it should be safe - but an ARRAY_RANGE_REF could be nicer than a memcpy, and the VIEW_CONVERT_EXPR could be inserted.
Comment 81 Mikael Morin 2012-08-01 18:37:55 UTC
(In reply to comment #79)
> If that's valid then you can make the middle-end happy by wrapping
> the RHS inside a VIEW_CONVERT_EXPR with the LHS type.
OK. will try.
I don't know yet though how to limit that to the very cases where it is necessary.


(In reply to comment #80)
> I have not closely looked at the dump, however,
>   this%y = this%find_y()
> means that one assigns component-wise the values from the RHS to the LHS; if
> there are pointer components, the pointer address is assigned; if there are
> allocatable components, those are - if needed - first (re)allocated and then
> (element-wise) assigned.
> 
> Thus, one only assigns the values and no pointers - and, hence, the RHS can be
> a nontarget while the LHS can be a target.
> 
Actually, there is a wrong-code bug about exactly this line (see bugs #47455 and #47586). But I think that even if there wasn't, the middle-end wouldn't be happy about it. The values are assigned component-wise, and that's the only way the middle-end would accept it.
Comment 82 Joost VandeVondele 2012-09-04 12:22:12 UTC
URL for the current version of the patch added.
Comment 83 Joost VandeVondele 2012-09-26 06:42:59 UTC
Mikael, any progress on this one (BTW, the PR is not yet assigned)? It would be great to have LTO work with Fortran in 4.8 (especially with all the inlining improvements). However, I would guess that this is stage 1 material, and I'm assuming stage 1 is nearing its end.
Comment 84 Tobias Burnus 2012-09-26 09:37:57 UTC
(In reply to comment #83)
> any progress on this one?

Well, the patch at http://gcc.gnu.org/ml/fortran/2012-08/msg00150.html solves several issues, but it still has issues, cf. http://gcc.gnu.org/ml/fortran/2012-08/msg00176.html

> It would be great to have LTO work with Fortran in 4.8

Well, it does for many codes; also the issue is merely* a tree-checking issues thus it should work* when GCC is compiled with release checking.

(* As the tree-checking failure is related to the "restrict" qualifier / pointer/target attribute, the issue of this PR might lead to wrong code. But seemingly, the issue doesn't surface in most codes.)
Comment 85 Mikael Morin 2012-09-26 16:06:59 UTC
(In reply to comment #84)
> (In reply to comment #83)
> > any progress on this one?
> 
> Well, the patch at http://gcc.gnu.org/ml/fortran/2012-08/msg00150.html solves
> several issues, but it still has issues, cf.
> http://gcc.gnu.org/ml/fortran/2012-08/msg00176.html

And some of the remaining issues are not present on trunk, so the patch is not a complete improvement.

I'm not actively working on this (nor anything else actually) any more.
Comment 86 Joost VandeVondele 2012-09-30 12:30:43 UTC
(In reply to comment #84)

LTO might work for many codes, as using allocatables in derived types was not standard Fortran90 (IIRC) and appears needed to trigger the bug. Anyway, since most people will use released versions of gcc, this checking error will be hidden behind --enable-checking=release. Only very few people will be able to locate and in particular reduce wrong code generation that only happens with LTO, so I wouldn't expect bug reports for actual wrong code generation very quickly.

Meanwhile a shorter testcase for 4.8, using gfortran -flto -O0.

  TYPE t
     REAL, DIMENSION(:), ALLOCATABLE :: r
  END TYPE t
  TYPE t_p
     TYPE(t), POINTER :: d_t
  END TYPE t_p

  REAL, DIMENSION(:), POINTER    :: d
  TYPE(t_p) ::  x
  d=>x%d_t%r
END
Comment 87 Peter Bergner 2012-11-15 22:46:23 UTC
I noticed this same error on the pr45586*.f90 test cases, but the reduced test case in Comment #86 ICEs for me too on powerpc64-linux.
Comment 88 Joost VandeVondele 2012-12-13 08:41:19 UTC
reconfirmed with current trunk.
Comment 89 Richard Biener 2012-12-13 12:06:57 UTC
Just to repeat, the ICEs are with checking enabled only (but possibly cover up
for wong-code).

I believe the correct solution will involve implementing the proposal for
introducing explicit restrict tags and using that in the fortran frontend
(removing the restrict qualification work).

See http://gcc.gnu.org/ml/gcc-patches/2011-10/msg01011.html.
Comment 90 Joost VandeVondele 2012-12-13 15:13:26 UTC
(In reply to comment #89)
> Just to repeat, the ICEs are with checking enabled only (but possibly cover up
> for wong-code).

I'm indeed worried that the release branches will as a result silently miscompile Fortran code in LTO mode, but I appreciate that the problem is hard to fix correctly. 

I wonder if an intermediate solution would be dropping the 'restrict qualifier' (in default of a better term) from allocatable components of derived types. This is a very small set of variables (as this was not allowed in Fortran90, IIRC) and should have small impact on the performance of typical programs. In exchange one would be able to use LTO without the risk of miscompilation, and presumably with significant benefit.
Comment 91 Richard Sandiford 2013-01-15 19:54:00 UTC
FWIW, fails for -mabi=32 on mips*-linux-gnu too (but not -mabi=n32
or -mabi=64).
Comment 92 Jakub Jelinek 2013-03-22 14:44:08 UTC
GCC 4.8.0 is being released, adjusting target milestone.
Comment 93 Joost VandeVondele 2013-03-24 08:04:17 UTC
reconfirmed with current (4.9) trunk. 4.8 branch now passes, obviously.

An unfortunate side-effect is that I can't test LTO in my trunk based, checking enabled, tester. So, I overlooked that LTO fails, for a different reason, to build CP2K (I'll try to file a separate PR in the near future for that).
Comment 94 Jakub Jelinek 2013-05-31 10:58:19 UTC
GCC 4.8.1 has been released.
Comment 95 Joost VandeVondele 2013-08-29 13:35:48 UTC
Reconfirmed the failure of the testcase from comment #86 with current trunk...
Comment 96 Jakub Jelinek 2013-10-16 09:48:55 UTC
GCC 4.8.2 has been released.
Comment 97 Joost VandeVondele 2013-11-21 18:29:53 UTC
(In reply to Richard Biener from comment #89)
> I believe the correct solution will involve implementing the proposal for
> introducing explicit restrict tags and using that in the fortran frontend
> (removing the restrict qualification work).
> 
> See http://gcc.gnu.org/ml/gcc-patches/2011-10/msg01011.html.

I guess that has now become:

http://gcc.gnu.org/ml/gcc-patches/2013-11/msg02727.html

progress!
Comment 98 David Kredba 2013-12-30 08:41:57 UTC
It was not commited yet, or was it please?
I still cannot compile scipy with -flto by trunk gcc.
Comment 99 Richard Biener 2014-01-09 11:52:46 UTC
Author: rguenth
Date: Thu Jan  9 11:52:43 2014
New Revision: 206461

URL: http://gcc.gnu.org/viewcvs?rev=206461&root=gcc&view=rev
Log:
2014-01-09  Richard Biener  <rguenther@suse.de>

	PR lto/45586
	* lto.c (hash_canonical_type): Do not hash TREE_ADDRESSABLE,
	TYPE_ALIGN, TYPE_RESTRICT or TYPE_REF_CAN_ALIAS_ALL.
	(gimple_canonical_types_compatible_p): Do not compare them either.

Modified:
    trunk/gcc/lto/ChangeLog
    trunk/gcc/lto/lto.c
Comment 100 Richard Biener 2014-01-09 14:44:03 UTC
Fixed on trunk.
Comment 101 Paul Thomas 2014-02-08 12:47:14 UTC
(In reply to Richard Biener from comment #100)
> Fixed on trunk.

Hi Richard,

Do you intend to backport this to 4.8 or, come to that, is it even backportable?  If 'no' to either, I'll close it out (I am engaged in regression bashing this afternoon).

Cheers

Paul

PS Thanks to everybody for their intestinal fortitude and persistence in fixing this PR!
Comment 102 rguenther@suse.de 2014-02-09 12:57:31 UTC
On February 8, 2014 1:47:14 PM GMT+01:00, "pault at gcc dot gnu.org" <gcc-bugzilla@gcc.gnu.org> wrote:
>http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45586
>
>Paul Thomas <pault at gcc dot gnu.org> changed:
>
>           What    |Removed                     |Added
>----------------------------------------------------------------------------
>               CC|                            |pault at gcc dot gnu.org
>
>--- Comment #101 from Paul Thomas <pault at gcc dot gnu.org> ---
>(In reply to Richard Biener from comment #100)
>> Fixed on trunk.
>
>Hi Richard,
>
>Do you intend to backport this to 4.8 or, come to that, is it even
>backportable?  If 'no' to either, I'll close it out (I am engaged in
>regression
>bashing this afternoon).

I don't intend to backport this.

Richard.

>Cheers
>
>Paul
>
>PS Thanks to everybody for their intestinal fortitude and persistence
>in fixing
>this PR!
Comment 103 Joost VandeVondele 2014-02-16 07:00:30 UTC
(In reply to rguenther@suse.de from comment #102)
> On February 8, 2014 1:47:14 PM GMT+01:00, "pault at gcc dot gnu.org"
> >> Fixed on trunk.
> I don't intend to backport this.

Thus fixed for 4.9 & won't fix for earlier branches. Thanks!