Bug 77278 - Use LTO for libgfortran
Summary: Use LTO for libgfortran
Status: NEW
Alias: None
Product: gcc
Classification: Unclassified
Component: libfortran (show other bugs)
Version: 7.0
: P3 enhancement
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords: lto, missed-optimization
Depends on: 68649 68717
Blocks: fortran-lto
  Show dependency treegraph
 
Reported: 2016-08-17 07:28 UTC by Thomas Koenig
Modified: 2020-02-18 18:36 UTC (History)
5 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2016-08-20 00:00:00


Attachments
LTO tree dump from simple test case (128.94 KB, text/plain)
2019-06-03 16:54 UTC, Thomas Koenig
Details
Preprocessed source of library file for LTO mismatch (21.66 KB, text/plain)
2019-06-04 16:39 UTC, Thomas Koenig
Details
Patch to put all libgfortran functions into a namespace (1.07 KB, patch)
2019-06-17 16:39 UTC, Thomas Koenig
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Thomas Koenig 2016-08-17 07:28:02 UTC
In order to aid optimization, it could be beneficial to
build libgfortran as an lto-enabled library. There could be
a big win for functions being called via constant propagation,
especially for I/O and array operations.

With I/O, checking for large number of options at runtime could
be avoided.  Array operations could profit from cases where arrays
are contiguous, when the library needs to take non-contiguous cases
into account.

Some discussion starts here.

https://gcc.gnu.org/ml/fortran/2016-08/msg00069.html

What has been done so far to identify potential problems
was to manually add -flto -ffat-lto-objects to the Makefile
in libgfortran and compile a simple program with -flto.

This led to error messages like

lto1: warning: type of '_gfortran_st_write' does not match original declaration [-Wlto-type-mismatch] ../../../trunk/libgfortran/io/transfer.c:3746:1: note: 'st_write' was previously declared here ../../../trunk/libgfortran/io/transfer.c:3746:1: note: code may be misoptimized unless -fno-strict-aliasing is used

so there is likely work to do on the library side and on the LTO side.
Comment 1 Thomas Koenig 2016-08-20 17:27:28 UTC
It is not easy to find a simple test case where an advantage
can be found.  With my hand-hacked LTO libgfortran, I get

ig25@linux-fd1f:~/Krempel/LTO> cat write.f90
program main
  implicit none
  integer :: i
  real(kind=8) :: a
  do i=1,10**7
    call random_number(a)
    write (10,'(E17.8," ")',advance="NO") a
  end do
end program main

With -static-libgfortran -lto I get for three consecutive runs

real    0m16.463s
user    0m15.979s
sys     0m0.449s

real    0m17.839s
user    0m17.440s
sys     0m0.388s

real    0m16.824s
user    0m16.378s
sys     0m0.436s

and with the normal shared library

real    0m19.508s
user    0m19.086s
sys     0m0.410s

real    0m19.850s
user    0m19.395s
sys     0m0.444s

real    0m18.213s
user    0m17.802s
sys     0m0.400s
Comment 2 Thomas Koenig 2016-08-21 09:31:18 UTC
Here's a test case which shows a performance loss with LTO-enabled
libgfortran.

program main
  implicit none
  integer, parameter :: n=10**7
  integer :: i
  real, dimension(n) :: a
  real :: t1, t2, t3
  call random_number(a)
  call cpu_time(t1)
  write (20,'(E17.12)') a
  call cpu_time(t2)
  write (21,'(E17.12)') (a(i),i=1,n)
  call cpu_time(t3)
  write (*,*) 'array write: ', t2-t1
  write (*,*) 'implied write: ', t3-t2
end program main

Running three times with shared libraries:
 array write:    12.0480003    
 implied write:    11.8389988    
 array write:    12.5640001    
 implied write:    12.5340004    
 array write:    11.8710003    
 implied write:    12.1729994  

Running three times with LTO:
 array write:    14.9330006    
 implied write:    15.3499994    
 array write:    14.2670002    
 implied write:    14.8539991    
 array write:    13.7550001    
 implied write:    14.8719997  

So, it is not always all that clear.
Comment 3 Thomas Koenig 2016-08-21 11:00:21 UTC
Some information about the type mismatch.

The first mismatch about gfortran_st_write is

lto1: warning: type of '_gfortran_st_write' does not match original declaration [-Wlto-type-mismatch]

Breakpoint 1, warn_types_mismatch (t1=0x7ffff6a8d150, t2=0x7ffff6a39540, loc1=27996512, loc2=0) at ../../trunk/gcc/ipa-devirt.c:1051
1051    {
(gdb) up
#1  0x0000000000654b39 in lto_symtab_merge_decls_2 (diagnosed_p=false, first=<optimized out>) at ../../trunk/gcc/lto/lto-symtab.c:679
679                                        DECL_SOURCE_LOCATION (decl));
(gdb) down
#0  warn_types_mismatch (t1=0x7ffff6a8d150, t2=0x7ffff6a39540, loc1=27996512, loc2=0) at ../../trunk/gcc/ipa-devirt.c:1051
1051    {
(gdb) call debug_tree(t1)
 <function_type 0x7ffff6a8d150
    type <void_type 0x7ffff6c50150 void VOID
        align 8 symtab 0 alias set -1 structural equality
        pointer_to_this <pointer_type 0x7ffff6c502a0>>
    QI
    size <integer_cst 0x7ffff6c37ca8 type <integer_type 0x7ffff6c3b2a0 bitsizetype> constant 8>
    unit size <integer_cst 0x7ffff6c37cc0 type <integer_type 0x7ffff6c3b1f8 sizetype> constant 1>
    align 8 symtab 0 alias set -1 structural equality
    arg-types <tree_list 0x7ffff6a873e8
        value <pointer_type 0x7ffff6a86738 type <record_type 0x7ffff6a867e0 st_parameter_dt>
            unsigned DI
            size <integer_cst 0x7ffff6c37bb8 constant 64>
            unit size <integer_cst 0x7ffff6c37bd0 constant 8>
            align 64 symtab 0 alias set 1 structural equality>
        chain <tree_list 0x7ffff6c499b0 value <void_type 0x7ffff6c50150 void>>>
    pointer_to_this <pointer_type 0x7ffff6aa19d8>>
(gdb) call debug_tree(t2)
 <function_type 0x7ffff6a39540
    type <void_type 0x7ffff6c50150 void VOID
        align 8 symtab 0 alias set -1 structural equality
        pointer_to_this <pointer_type 0x7ffff6c502a0>>
    QI
    size <integer_cst 0x7ffff6c37ca8 type <integer_type 0x7ffff6c3b2a0 bitsizetype> constant 8>
    unit size <integer_cst 0x7ffff6c37cc0 type <integer_type 0x7ffff6c3b1f8 sizetype> constant 1>
    align 8 symtab 0 alias set -1 structural equality
    attributes <tree_list 0x7ffff6a3baf0
        purpose <identifier_node 0x7ffff6c69af0 fn spec>
        value <tree_list 0x7ffff6a3bac8
            value <string_cst 0x7ffff6e26dc8 constant ".w">>>
    arg-types <tree_list 0x7ffff6a3baa0
        value <pointer_type 0x7ffff6a39498 type <record_type 0x7ffff6a393f0 __st_parameter_dt>
            unsigned DI
            size <integer_cst 0x7ffff6c37bb8 constant 64>
            unit size <integer_cst 0x7ffff6c37bd0 constant 8>
            align 64 symtab 0 alias set 6 structural equality>
        chain <tree_list 0x7ffff6c499b0 value <void_type 0x7ffff6c50150 void>>>
    pointer_to_this <pointer_type 0x7ffff6a44150>>

which looks quite different, but I lack the tree-foo to really decypher this.
Comment 4 Dominique d'Humieres 2016-08-21 11:09:12 UTC
> This led to error messages like
>
> lto1: warning: type of '_gfortran_st_write' does not match original
> declaration [-Wlto-type-mismatch] ../../../trunk/libgfortran/io/transfer.c:3746:1:
> note: 'st_write' was previously declared here
> ../../../trunk/libgfortran/io/transfer.c:3746:1: note: code may be
> misoptimized unless -fno-strict-aliasing is used
>
> so there is likely work to do on the library side and on the LTO side.

This looks like pr68649 and pr68717.
Comment 5 Thomas Koenig 2019-06-02 17:07:05 UTC
One thing we would also have to tackle is GFC_LOGICAL arguments.
C only has one bool type, which is (for gcc) equivalent to
logical(kind=1).  We might just get by with 

typedef enum { _zero=1, _one=1 } GFC_LOGICAL_4;

but what about arguments with other logical types?
We might actually need a C extension there, or disable
aliasing-based optimization.
Comment 6 Thomas Koenig 2019-06-02 21:54:18 UTC
So, I played around with this a little. By putting in
-flto and -ffat-lto-binaries into CFLAGS, FFLAGS and LDFLAGS
into the Makefile in the libgfortran build directory, it is
possible to build an LTO-enabled libgfortran.

For the first test (write.f90 from comment#1 below), a small
patch

Index: io/open.c
===================================================================
--- io/open.c   (Revision 271843)
+++ io/open.c   (Arbeitskopie)
@@ -740,6 +740,7 @@ st_open (st_parameter_open *opp)
   GFC_INTEGER_4 cf = opp->common.flags;
   unit_convert conv;
  
+  memset (&flags, 0, sizeof(flags));
   library_start (&opp->common);
 
   /* Decode options.  */

led to a lot of conditional code not being pulled into the
main program. So far, so good - constant folding for open
was good.

The main function then became (in the optimized tree dump)

  <bb 2> [local count: 10737418]:
  open_parm.0.common.filename = &"write.f90"[1]{lb: 1 sz: 1};
  open_parm.0.common.line = 5;
  open_parm.0.file = &"foo.bar"[1]{lb: 1 sz: 1};
  open_parm.0.file_len = 7;
  open_parm.0.readonly = 0;
  MEM[(int *)&open_parm.0] = 42966450432;
  st_open.constprop (&open_parm.0);
  open_parm.0 ={v} {CLOBBER};
  # DEBUG i => 1

  <bb 3> [local count: 1063004407]:
  # ivtmp_27 = PHI <10000000(2), ivtmp_1(3)>
  # DEBUG i => NULL
  _gfortran_random_r8 (&a);
  dt_parm.1.common.filename = &"write.f90"[1]{lb: 1 sz: 1};
  dt_parm.1.common.line = 8;
  dt_parm.1.advance = &"NO"[1]{lb: 1 sz: 1};
  dt_parm.1.format = &"(E17.8,\" \")"[1]{lb: 1 sz: 1};
  MEM[(long int *)&dt_parm.1 + 88B] = { 11, 2 };
  MEM[(int *)&dt_parm.1] = 42949685248;
  st_write.constprop (&dt_parm.1);
  transfer_real_write.constprop (&dt_parm.1, &a);
  st_write_done (&dt_parm.1);
  dt_parm.1 ={v} {CLOBBER};
  # DEBUG i => NULL
  ivtmp_1 = ivtmp_27 + 4294967295;
  if (ivtmp_1 == 0)
    goto <bb 4>; [1.01%]
  else
    goto <bb 3>; [98.99%]

  <bb 4> [local count: 10737418]:
  a ={v} {CLOBBER};
  return;

}

So, dt_parm_1 is still filled with information in the tight loop
(which the library does not change), and the call to
transfer_real_write.constprop does not do a lot of the things
that could be done in theory, for example keeping the unit
number cached, take a note that this is not asynchronous,
that we always use "NO" on advance in the loop, etc.

So, is it realistic to expect that LTO could do this kind
of thing with the very complex structure that libgfortran?
Comment 7 Richard Biener 2019-06-03 08:39:51 UTC
(In reply to Thomas Koenig from comment #5)
> One thing we would also have to tackle is GFC_LOGICAL arguments.
> C only has one bool type, which is (for gcc) equivalent to
> logical(kind=1).  We might just get by with 
> 
> typedef enum { _zero=1, _one=1 } GFC_LOGICAL_4;
> 
> but what about arguments with other logical types?
> We might actually need a C extension there, or disable
> aliasing-based optimization.

aliasing shouldn't be an issue here.  The other logical kinds need to
be mapped to short/int/long anyways for ABI reasons, no?
Comment 8 rguenther@suse.de 2019-06-03 08:42:08 UTC
On Sun, 2 Jun 2019, tkoenig at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77278
> 
> --- Comment #6 from Thomas Koenig <tkoenig at gcc dot gnu.org> ---
...
> So, dt_parm_1 is still filled with information in the tight loop
> (which the library does not change), and the call to
> transfer_real_write.constprop does not do a lot of the things
> that could be done in theory, for example keeping the unit
> number cached, take a note that this is not asynchronous,
> that we always use "NO" on advance in the loop, etc.
> 
> So, is it realistic to expect that LTO could do this kind
> of thing with the very complex structure that libgfortran?

Sure, why not.  Of course the original motivation wasn't so
much I/O but inlining/optimizing intrinsics.  The frontend
does a lot more inlining itself here today so the effect
might not be as big.
Comment 9 Thomas Koenig 2019-06-03 16:54:10 UTC
Created attachment 46446 [details]
LTO tree dump from simple test case

So, I tried out a simple program:

$ cat minloc.f90
program main
  real, dimension(10,10) :: a
  integer, dimension(2) :: m1
  call random_number(a)
  m1 = minloc(a)
  print *,m1
end program main

Compiling this with the fat-object libgfortran results in the somewhat
humorous

$ gfortran -fdump-tree-optimized -O3 -flto -static-libgfortran  minloc.f90
minloc.f90:5: warning: type of '_gfortran_minloc0_4_r4' does not match original declaration [-Wlto-type-mismatch]
    5 |   m1 = minloc(a)
      | 
../../../ggdb3/libgfortran/generated/minloc0_4_r4.c:38:1: note: type mismatch in parameter 3
../../../ggdb3/libgfortran/generated/minloc0_4_r4.c:38:1: note: 'minloc0_4_r4' was previously declared here
../../../ggdb3/libgfortran/generated/minloc0_4_r4.c:38:1: note: code may be misoptimized unless '-fno-strict-aliasing' is used

which looks like an LTO bug.

There is a lot of code pulled in for writing to standard output
which could be optimized away.

The good news: Inlining _gfortran_minloc0_4_r4 appears to work :-)
Comment 10 rguenther@suse.de 2019-06-03 18:16:05 UTC
On June 3, 2019 6:54:10 PM GMT+02:00, "tkoenig at gcc dot gnu.org" <gcc-bugzilla@gcc.gnu.org> wrote:
>https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77278
>
>--- Comment #9 from Thomas Koenig <tkoenig at gcc dot gnu.org> ---
>Created attachment 46446 [details]
>  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=46446&action=edit
>LTO tree dump from simple test case
>
>So, I tried out a simple program:
>
>$ cat minloc.f90
>program main
>  real, dimension(10,10) :: a
>  integer, dimension(2) :: m1
>  call random_number(a)
>  m1 = minloc(a)
>  print *,m1
>end program main
>
>Compiling this with the fat-object libgfortran results in the somewhat
>humorous
>
>$ gfortran -fdump-tree-optimized -O3 -flto -static-libgfortran 
>minloc.f90
>minloc.f90:5: warning: type of '_gfortran_minloc0_4_r4' does not match
>original
>declaration [-Wlto-type-mismatch]
>    5 |   m1 = minloc(a)
>      | 
>../../../ggdb3/libgfortran/generated/minloc0_4_r4.c:38:1: note: type
>mismatch
>in parameter 3
>../../../ggdb3/libgfortran/generated/minloc0_4_r4.c:38:1: note:
>'minloc0_4_r4'
>was previously declared here
>../../../ggdb3/libgfortran/generated/minloc0_4_r4.c:38:1: note: code
>may be
>misoptimized unless '-fno-strict-aliasing' is used
>
>which looks like an LTO bug.

It tells you the actual argument of the Fortran frontend generated call is not compatible with the C prototype. It
Doesn't say which types are involved here and the leading sentence is confusing. 

>There is a lot of code pulled in for writing to standard output
>which could be optimized away.
>
>The good news: Inlining  _gfortran_minloc0_4_r4 appears to work :-)
Comment 11 Richard Biener 2019-06-04 06:57:45 UTC
Honza can probably suggest ways to make the warning more to the point and how to show the missing information (the actual argument type vs. the declared one).
Comment 12 Thomas Koenig 2019-06-04 07:32:36 UTC
In libgfortran, we have

#define GFC_ARRAY_DESCRIPTOR(type) \
struct {\
  type *base_addr;\
  size_t offset;\
  dtype_type dtype;\
  index_type span;\
  descriptor_dimension dim[];\
}

and then later

typedef GFC_ARRAY_DESCRIPTOR (GFC_INTEGER_4) gfc_array_i4;

so the array descriptors expected by the libgfotran routines
have a flexible array members.

If, in the front end, we have the equivalent of (the type name
isn't exactly what the front end generates)

typedef struct {
  GFC_INTEGER_4 *base_addr;\
  size_t offset;\
  dtype_type dtype;\
  index_type span;\
  descriptor_dimension dim[3];\
} _array03_integer_4_descriptor;

_array03_integer_4_descriptor my_descriptor;

and a pointer type that corresponds to what the library
expects, we should then be able to call

    minloc_... ((gfc_array_i4 *) &my_descriptor, ..)

right?

I think this should probably be restricted to calling the
library, I would feel nervous touching use code with this.
Comment 13 rguenther@suse.de 2019-06-04 08:23:49 UTC
On Tue, 4 Jun 2019, tkoenig at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77278
> 
> --- Comment #12 from Thomas Koenig <tkoenig at gcc dot gnu.org> ---
> In libgfortran, we have
> 
> #define GFC_ARRAY_DESCRIPTOR(type) \
> struct {\
>   type *base_addr;\
>   size_t offset;\
>   dtype_type dtype;\
>   index_type span;\
>   descriptor_dimension dim[];\
> }
> 
> and then later
> 
> typedef GFC_ARRAY_DESCRIPTOR (GFC_INTEGER_4) gfc_array_i4;
> 
> so the array descriptors expected by the libgfotran routines
> have a flexible array members.
> 
> If, in the front end, we have the equivalent of (the type name
> isn't exactly what the front end generates)
> 
> typedef struct {
>   GFC_INTEGER_4 *base_addr;\
>   size_t offset;\
>   dtype_type dtype;\
>   index_type span;\
>   descriptor_dimension dim[3];\
> } _array03_integer_4_descriptor;
> 
> _array03_integer_4_descriptor my_descriptor;

Yeah, I do remember this.  I think we settled on the above
(previously you had dim[7] in the library I think) to be
compatible.  Still a C simple testcase complains:

typedef struct { int ndim; int dim[]; } *descp;
void foo (descp d);

void bar()
{
  struct { int ndim; int dim[2]; } desc;
  desc.ndim = 2;
  foo (&desc);
}

t2.c: In function ‘bar’:
t2.c:8:8: warning: passing argument 1 of ‘foo’ from incompatible pointer 
type [-Wincompatible-pointer-types]
   foo (&desc);
        ^~~~~
t2.c:2:17: note: expected ‘descp’ {aka ‘struct <anonymous> *’} but 
argument is of type ‘struct <anonymous> *’
 void foo (descp d);
           ~~~~~~^

and we probably assign different alias sets to both.

Now to make aliasing happy both the Frontend and LTO have to
compute the same TYPE_CANONICAL for _all_ of the array
descriptor types.  You can either go and allocate
dim always to the max size statically or in the Fortran
FE use self-referential types (not sure if you then can
statically instantiate an object of such type...) or
rewrite all accesses to the fixed-dimension statically
allocated array descriptors to go via the dim[] type
(I think I suggested the latter elsewhere).

So instantiate my_descriptor and then store for further
use VIEW_CONVERT_EXPR <generic-descriptor-type> (my_descriptor).

I hope that doesn't defeat [IPA] optimization ...

> and a pointer type that corresponds to what the library
> expects, we should then be able to call
> 
>     minloc_... ((gfc_array_i4 *) &my_descriptor, ..)
> 
> right?
> 
> I think this should probably be restricted to calling the
> library, I would feel nervous touching use code with this.

While that might silence the warning it doesn't solve the
TBAA issue that is indeed present -- the populating of
the descriptor on the fortran caller side would not
alias with reads from the passed descriptor done via
the C routine and its descriptor type.  In practice we
probably see the must-alias relationship and let the
TBAA disambiguation possibility unused but I guess we'll
quickly get less lucky...
Comment 14 Jan Hubicka 2019-06-04 09:12:53 UTC
> 
> Yeah, I do remember this.  I think we settled on the above
> (previously you had dim[7] in the library I think) to be
> compatible.  Still a C simple testcase complains:
> 
> typedef struct { int ndim; int dim[]; } *descp;
> void foo (descp d);
> 
> void bar()
> {
>   struct { int ndim; int dim[2]; } desc;
>   desc.ndim = 2;
>   foo (&desc);
> }
> 
> t2.c: In function ‘bar’:
> t2.c:8:8: warning: passing argument 1 of ‘foo’ from incompatible pointer 
> type [-Wincompatible-pointer-types]
>    foo (&desc);
>         ^~~~~
> t2.c:2:17: note: expected ‘descp’ {aka ‘struct <anonymous> *’} but 
> argument is of type ‘struct <anonymous> *’
>  void foo (descp d);
>            ~~~~~~^
> 
> and we probably assign different alias sets to both.

Curiously enough in C version of the LTO testcase I get no warning now
since we simplify function type and thus both pointers are turned into
incomplete pointers and considered TBAA compatible.
After lunch I will check why the warning triggers here.

Still the alias set is different, so I think we more or less get lucky
by using base+offset tests first since array descriptors are accessed
directly after constant propagation of poointers.
> 
> Now to make aliasing happy both the Frontend and LTO have to
> compute the same TYPE_CANONICAL for _all_ of the array
> descriptor types.  You can either go and allocate
> dim always to the max size statically or in the Fortran
> FE use self-referential types (not sure if you then can
> statically instantiate an object of such type...) or
> rewrite all accesses to the fixed-dimension statically
> allocated array descriptors to go via the dim[] type
> (I think I suggested the latter elsewhere).
> 
> So instantiate my_descriptor and then store for further
> use VIEW_CONVERT_EXPR <generic-descriptor-type> (my_descriptor).
> 
> I hope that doesn't defeat [IPA] optimization ...
I think Martin's code generally gives up on type mismaches so it
is quite possible it gives up already.

An option would be also to teach LTO that array descriptors are
special types aliasing with each other but not aliasing with
anything else.

Honza
Comment 15 rguenther@suse.de 2019-06-04 09:56:12 UTC
On Tue, 4 Jun 2019, hubicka at ucw dot cz wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77278
> 
> --- Comment #14 from Jan Hubicka <hubicka at ucw dot cz> ---
> > 
> > Yeah, I do remember this.  I think we settled on the above
> > (previously you had dim[7] in the library I think) to be
> > compatible.  Still a C simple testcase complains:
> > 
> > typedef struct { int ndim; int dim[]; } *descp;
> > void foo (descp d);
> > 
> > void bar()
> > {
> >   struct { int ndim; int dim[2]; } desc;
> >   desc.ndim = 2;
> >   foo (&desc);
> > }
> > 
> > t2.c: In function ‘bar’:
> > t2.c:8:8: warning: passing argument 1 of ‘foo’ from incompatible pointer 
> > type [-Wincompatible-pointer-types]
> >    foo (&desc);
> >         ^~~~~
> > t2.c:2:17: note: expected ‘descp’ {aka ‘struct <anonymous> *’} but 
> > argument is of type ‘struct <anonymous> *’
> >  void foo (descp d);
> >            ~~~~~~^
> > 
> > and we probably assign different alias sets to both.
> 
> Curiously enough in C version of the LTO testcase I get no warning now
> since we simplify function type and thus both pointers are turned into
> incomplete pointers and considered TBAA compatible.
> After lunch I will check why the warning triggers here.
> 
> Still the alias set is different, so I think we more or less get lucky
> by using base+offset tests first since array descriptors are accessed
> directly after constant propagation of poointers.
> > 
> > Now to make aliasing happy both the Frontend and LTO have to
> > compute the same TYPE_CANONICAL for _all_ of the array
> > descriptor types.  You can either go and allocate
> > dim always to the max size statically or in the Fortran
> > FE use self-referential types (not sure if you then can
> > statically instantiate an object of such type...) or
> > rewrite all accesses to the fixed-dimension statically
> > allocated array descriptors to go via the dim[] type
> > (I think I suggested the latter elsewhere).
> > 
> > So instantiate my_descriptor and then store for further
> > use VIEW_CONVERT_EXPR <generic-descriptor-type> (my_descriptor).
> > 
> > I hope that doesn't defeat [IPA] optimization ...
> I think Martin's code generally gives up on type mismaches so it
> is quite possible it gives up already.
> 
> An option would be also to teach LTO that array descriptors are
> special types aliasing with each other but not aliasing with
> anything else.

I'd rather not do that ;)  Btw, I wonder what happens at
the call boundary inside a single fortran module where
the caller passes a dim[2] array to a subroutine
handling arbitrary dimension arrays?  I suspect the
IL would have the very same TBAA issue.  Can you produce
a fortran testcase that exposes such a case so we can have a
look into the details?
Comment 16 Martin Jambor 2019-06-04 10:38:23 UTC
Hi,

On Tue, Jun 04 2019, Jan Hubicka wrote:
>> 
>> Yeah, I do remember this.  I think we settled on the above
>> (previously you had dim[7] in the library I think) to be
>> compatible.  Still a C simple testcase complains:
>> 
>> typedef struct { int ndim; int dim[]; } *descp;
>> void foo (descp d);
>> 
>> void bar()
>> {
>>   struct { int ndim; int dim[2]; } desc;
>>   desc.ndim = 2;
>>   foo (&desc);
>> }
>> 
>> t2.c: In function ‘bar’:
>> t2.c:8:8: warning: passing argument 1 of ‘foo’ from incompatible pointer 
>> type [-Wincompatible-pointer-types]
>>    foo (&desc);
>>         ^~~~~
>> t2.c:2:17: note: expected ‘descp’ {aka ‘struct <anonymous> *’} but 
>> argument is of type ‘struct <anonymous> *’
>>  void foo (descp d);
>>            ~~~~~~^
>> 
>> and we probably assign different alias sets to both.
>
> Curiously enough in C version of the LTO testcase I get no warning now
> since we simplify function type and thus both pointers are turned into
> incomplete pointers and considered TBAA compatible.
> After lunch I will check why the warning triggers here.
>
> Still the alias set is different, so I think we more or less get lucky
> by using base+offset tests first since array descriptors are accessed
> directly after constant propagation of poointers.
>> 
>> Now to make aliasing happy both the Frontend and LTO have to
>> compute the same TYPE_CANONICAL for _all_ of the array
>> descriptor types.  You can either go and allocate
>> dim always to the max size statically or in the Fortran
>> FE use self-referential types (not sure if you then can
>> statically instantiate an object of such type...) or
>> rewrite all accesses to the fixed-dimension statically
>> allocated array descriptors to go via the dim[] type
>> (I think I suggested the latter elsewhere).
>> 
>> So instantiate my_descriptor and then store for further
>> use VIEW_CONVERT_EXPR <generic-descriptor-type> (my_descriptor).
>> 
>> I hope that doesn't defeat [IPA] optimization ...
> I think Martin's code generally gives up on type mismaches so it
> is quite possible it gives up already.

I guess I'm missing some context here (and the example above does not
contain t1.c?).  But I quickly grepped for typec_compatible_p and
useless_type_conversion_p in ipa-prop.c (and ipa-cp.c) and the code
tries to resolve issues with inserting V_C_E rather than giving up
straight away.

Martin
Comment 17 Jan Hubicka 2019-06-04 11:26:39 UTC
A small self-contained example would be welcome, I can take a look why aliasing oracle does not mess things up.

Concerning the warning, those are quite hard to do - the line information should point to mismatched declarations, but does not since libgfortran is no longer on the expected path.

Dumping actual type will get it output in C-like syntax which is probably not very useful except for debugging reasons...
Comment 18 Thomas Koenig 2019-06-04 16:39:11 UTC
Created attachment 46451 [details]
Preprocessed source of library file for LTO mismatch

Hi,

here is a test case (preprocessed source from libgfortran).  To reproduce,
use the test program

$ cat minloc.f90
program main
  real, dimension(10,10) :: a
  integer, dimension(2) :: m1
  call random_number(a)
  m1 = minloc(a)
  print *,m1
end program main

and compile/link with

$ gfortran -static-libgfortran -flto -O3 minloc.f90 minloc0_4_r4.i 
minloc.f90:5: warning: type of '_gfortran_minloc0_4_r4' does not match original declaration [-Wlto-type-mismatch]
    5 |   m1 = minloc(a)
      | 
../../../ggdb3/libgfortran/generated/minloc0_4_r4.c:38:1: note: type mismatch in parameter 3
../../../ggdb3/libgfortran/generated/minloc0_4_r4.c:38:1: note: 'minloc0_4_r4' was previously declared here
../../../ggdb3/libgfortran/generated/minloc0_4_r4.c:38:1: note: code may be misoptimized unless '-fno-strict-aliasing' is used
Comment 19 Thomas Koenig 2019-06-04 16:46:46 UTC
(In reply to rguenther@suse.de from comment #15)
  Btw, I wonder what happens at
> the call boundary inside a single fortran module where
> the caller passes a dim[2] array to a subroutine
> handling arbitrary dimension arrays?  I suspect the
> IL would have the very same TBAA issue.  Can you produce
> a fortran testcase that exposes such a case so we can have a
> look into the details?

Here is a test case:

module x
  implicit none
contains
  subroutine foo(a)
    real, dimension(..) :: a
    print *,shape(a)
  end subroutine foo
  subroutine bar
    real, dimension(2,2) :: a
    real, dimension(3,3,3) :: b
    call foo(a)
    call foo(b)
  end subroutine bar
end module x

program main
  use x
  call bar
end program main

Looking at the *.original tree dump, we have

bar ()
{
  real(kind=4) a[4];
  real(kind=4) b[27];

  {
    struct array02_real(kind=4) parm.0;

[...]

    foo (&parm.0);
  }

  {
    struct array03_real(kind=4) parm.1;

[...]

    foo (&parm.1);
  }

and

foo (struct array15_real(kind=4) & restrict a)
{
  {
    struct __st_parameter_dt dt_parm.2;

This does not really look very healthy (but it is not warned about).
Comment 20 Jan Hubicka 2019-06-04 17:24:56 UTC
OK, the mismatched declaration types are:
void <T67d> (struct array01_integer(kind=4) &, float & restrict, logical(kind=4) *)
and
void <T621> (struct gfc_array_i4 * restrict, struct gfc_array_r4 * restrict, GFC_LOGICAL_4)

The mismatch happens in the last parameter that is logical(kind=4) and GFC_LOGICAL_4.

 <pointer_type 0x7ffff6616d20
    type <boolean_type 0x7ffff6616c78 logical(kind=4) unsigned SI
        size <integer_cst 0x7ffff680cdb0 constant 32>
        unit-size <integer_cst 0x7ffff680cdc8 constant 4>
        align:32 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type 0x7ffff6616c78 precision:1 min <integer_cst 0x7ffff6a088e8 0> max <integer_cst 0x7ffff6a08900 1>
        pointer_to_this <pointer_type 0x7ffff6616d20>>
    unsigned DI
    size <integer_cst 0x7ffff680cb70 type <integer_type 0x7ffff68210a8 bitsizetype> constant 64>
    unit-size <integer_cst 0x7ffff680cb88 type <integer_type 0x7ffff6821000 sizetype> constant 8>
    align:64 warn_if_not_align:0 symtab:0 alias-set 3 structural-equality>


 <enumeral_type 0x7ffff6a06f18 GFC_LOGICAL_4 unsigned SI
    size <integer_cst 0x7ffff680cdb0 type <integer_type 0x7ffff68210a8 bitsizetype> constant 32>
    unit-size <integer_cst 0x7ffff680cdc8 type <integer_type 0x7ffff6821000 sizetype> constant 4>
    align:32 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type 0x7ffff6821690 precision:32 min <integer_cst 0x7ffff680cde0 0> max <integer_cst 0x7ffff680cd98 4294967295> context <translation_unit_decl 0x7ffff68161e0 ../../../ggdb3/libgfortran/generated/minloc0_4_r4.c>
    pointer_to_this <pointer_type 0x7ffff6616690>>

So mixing up enum and pointer to enum.
Fixing C source to expect pointer to enum makes warning to go away, but looking at the gimple produced, it really just seems in bug in fortran FE declaring the function incorrectly? It seems to really just pass 0 instead of pointer to 0:
_gfortran_minloc0_4_r4 (&parm.1, _40, 0);

Honza
Comment 21 Thomas Koenig 2019-06-05 05:21:22 UTC
(In reply to Jan Hubicka from comment #20)
> OK, the mismatched declaration types are:
> void <T67d> (struct array01_integer(kind=4) &, float & restrict,
> logical(kind=4) *)
> and
> void <T621> (struct gfc_array_i4 * restrict, struct gfc_array_r4 * restrict,
> GFC_LOGICAL_4)
> 
> The mismatch happens in the last parameter that is logical(kind=4) and
> GFC_LOGICAL_4.

Thanks for looking into this.

One question: What do I have to do to get at this type of information,
and the following tree type dump?  What exactly would I have to
run?  (I looked around, but could not find any info).
 

> So mixing up enum and pointer to enum.

> Fixing C source to expect pointer to enum makes warning to go away, but
> looking at the gimple produced, it really just seems in bug in fortran FE
> declaring the function incorrectly? It seems to really just pass 0 instead
> of pointer to 0:
> _gfortran_minloc0_4_r4 (&parm.1, _40, 0);


It was certainly the inention to pass a value.

However, I tried doing this with this little patchlet

Index: intrinsic.c
===================================================================
--- intrinsic.c (Revision 271843)
+++ intrinsic.c (Arbeitskopie)
@@ -2614,6 +2614,8 @@ add_functions (void)
               msk, BT_LOGICAL, dl, OPTIONAL, kind, BT_INTEGER, di, OPTIONAL,
               bck, BT_LOGICAL, dl, OPTIONAL);
 
+  set_attr_value (5, false, false, false, false, true);
+
   make_generic ("minloc", GFC_ISYM_MINLOC, GFC_STD_F95);
 
   add_sym_3red ("minval", GFC_ISYM_MINVAL, CLASS_TRANSFORMATIONAL, ACTUAL_NO, BT_REAL, dr, GFC_STD_F95,

and ended up with

minloc.f90:5: warning: type of '_gfortran_minloc0_4_r4' does not match original declaration [-Wlto-type-mismatch]
    5 |   m1 = minloc(a)
      | 
../../../ggdb3/libgfortran/generated/minloc0_4_r4.c:38:1: note: type mismatch in parameter 3
../../../ggdb3/libgfortran/generated/minloc0_4_r4.c:38:1: note: type 'GFC_LOGICAL_4' should match type 'logical(kind=4)'
../../../ggdb3/libgfortran/generated/minloc0_4_r4.c:38:1: note: 'minloc0_4_r4' was previously declared here
../../../ggdb3/libgfortran/generated/minloc0_4_r4.c:38:1: note: code may be misoptimized unless '-fno-strict-aliasing' is used

Now, currently GFC_LOGICAL_4 is just a typedef for GFC_INTEGER_4, which
in turn is just a typedef for int.  Could this be the source of the
trouble here?  And what could we do about a GFC_LOGICAL_2 and so on?
Comment 22 rguenther@suse.de 2019-06-05 07:46:33 UTC
On Tue, 4 Jun 2019, tkoenig at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77278
> 
> --- Comment #19 from Thomas Koenig <tkoenig at gcc dot gnu.org> ---
> (In reply to rguenther@suse.de from comment #15)
>   Btw, I wonder what happens at
> > the call boundary inside a single fortran module where
> > the caller passes a dim[2] array to a subroutine
> > handling arbitrary dimension arrays?  I suspect the
> > IL would have the very same TBAA issue.  Can you produce
> > a fortran testcase that exposes such a case so we can have a
> > look into the details?
> 
> Here is a test case:
> 
> module x
>   implicit none
> contains
>   subroutine foo(a)
>     real, dimension(..) :: a
>     print *,shape(a)
>   end subroutine foo
>   subroutine bar
>     real, dimension(2,2) :: a
>     real, dimension(3,3,3) :: b
>     call foo(a)
>     call foo(b)
>   end subroutine bar
> end module x
> 
> program main
>   use x
>   call bar
> end program main
> 
> Looking at the *.original tree dump, we have
> 
> bar ()
> {
>   real(kind=4) a[4];
>   real(kind=4) b[27];
> 
>   {
>     struct array02_real(kind=4) parm.0;
> 
> [...]
> 
>     foo (&parm.0);
>   }
> 
>   {
>     struct array03_real(kind=4) parm.1;
> 
> [...]
> 
>     foo (&parm.1);
>   }
> 
> and
> 
> foo (struct array15_real(kind=4) & restrict a)
> {
>   {
>     struct __st_parameter_dt dt_parm.2;
> 
> This does not really look very healthy (but it is not warned about).

OK, so changing the testcase to

module x
  implicit none
contains
  subroutine foo(a)
    real, dimension(..) :: a
    if (rank(a) < 2) STOP 3
  end subroutine foo
  subroutine bar
    real, dimension(2,2) :: a
    real, dimension(3,3,3) :: b
    call foo(a)
    call foo(b)
  end subroutine bar
end module x

program main
  use x
  call bar
end program main

makes us inline foo() where we then see

  struct array03_real(kind=4) parm.1;
  struct array02_real(kind=4) parm.0;
...
  parm.0.dtype.rank = 2;
...
  _15 = MEM[(struct array15_real(kind=4) &)&parm.0].dtype.rank;
  if (_15 <= 1)

which shows a possible wrong-code issue because the store
to rank is via array02 but the read uses array15.  We don't
miscompile this because we try to be forgiving if we see
a must-alias.

The frontend should create a descriptor type mimicing the one
in the library API and emit

  MEM[(struct arrayN_real(kind=4) &)&param.0].dtype.rank = 2;
...
  _15 = MEM[(struct arrayN_real(kind=4) &)&param.0].dtype.rank;
  if (_15 <= 1)

that is, all accesses to array descriptors should be as-if
they were indirect accesses through the assumed rank descriptor
type with the flex-array dim member.

Note this would no longer allow TBAA disambiguation of
different rank array descriptor accesses.  So alternatively
we would have to arrange accesses via structs with a flex
array member alias accesses via structs with fixed-array
members.  Still functions accessing assumed rank arrays
like foo() above then need to use the flexarray descriptor
type to access dtype.rank, not as it does at the moment
the max-rank descriptor type.  I suppose that would be
the first thing to fix (maybe also the easiest).

Honza and me can then think about ways to make
struct { int ndim; int dim[]; } accesses conflict with
struct { int ndim; int dim[N]; } with arbitrary N.
Like in record-component-aliases if it sees a trailing
fixed-size array member, build the corresponding flex-array
record and record a subset of that (ugh).
Comment 23 Jan Hubicka 2019-06-05 08:15:25 UTC
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77278
> 
> --- Comment #21 from Thomas Koenig <tkoenig at gcc dot gnu.org> ---
> (In reply to Jan Hubicka from comment #20)
> > OK, the mismatched declaration types are:
> > void <T67d> (struct array01_integer(kind=4) &, float & restrict,
> > logical(kind=4) *)
> > and
> > void <T621> (struct gfc_array_i4 * restrict, struct gfc_array_r4 * restrict,
> > GFC_LOGICAL_4)
> > 
> > The mismatch happens in the last parameter that is logical(kind=4) and
> > GFC_LOGICAL_4.
> 
> Thanks for looking into this.
> 
> One question: What do I have to do to get at this type of information,
> and the following tree type dump?  What exactly would I have to
> run?  (I looked around, but could not find any info).

I simply add debug_tree and debug_generic_stmt to the place warning is
output (in lto/lto-symtab.c)
It would be nice to make these warnings more understandable somehow.  I
did some work on the ODR warnings, but in this case it is even harder
(especially because the mismatch is cross-language and we have no way to
dump types in user understandable form at this stage).
> 
> 
> > So mixing up enum and pointer to enum.
> 
> > Fixing C source to expect pointer to enum makes warning to go away, but
> > looking at the gimple produced, it really just seems in bug in fortran FE
> > declaring the function incorrectly? It seems to really just pass 0 instead
> > of pointer to 0:
> > _gfortran_minloc0_4_r4 (&parm.1, _40, 0);
> 
> Now, currently GFC_LOGICAL_4 is just a typedef for GFC_INTEGER_4, which
> in turn is just a typedef for int.  Could this be the source of the
> trouble here?  And what could we do about a GFC_LOGICAL_2 and so on?

In the dump it seems that fortran FE produces boolean_type while the
other is enum type.
We turn enum type into integer type, but boolean remains boolean
so they are not considered same.

Does changing typedef to boolean help?

Honza
> 
> -- 
> You are receiving this mail because:
> You are on the CC list for the bug.
Comment 24 Jan Hubicka 2019-06-05 11:48:03 UTC
Hi,
actually it won't help since C has only one bool type and not bools in different sizes (why would one need that?).
I guess it would be easiest to turn Fortran frontend to use integers here.

Honza
Comment 25 Thomas Koenig 2019-06-06 06:26:24 UTC
(In reply to Jan Hubicka from comment #24)
> Hi,
> actually it won't help since C has only one bool type and not bools in
> different sizes (why would one need that?).

"Because it's in the Fortran language standard" is probably a good
answer as any.  For example, the standard specifies that default
logical and default integer have to have the same size, and
it specifies all the other logical kinds.

> I guess it would be easiest to turn Fortran frontend to use integers here.

Easiest for this case, yes.

However, this is far from the only case where integer vs. logical
is a problem in libgfortran - to reduce code size, we've been treating
them interchangeably for quite some time.

Really, it would be easiest if there was a way to tell the middle
end that logical(kind=4) and integer(kind=4) could alias.

There is another, possibly even worse, case.  To reduce combinatorical
explosion of some functions, we have been using this idiom

  mask_kind = GFC_DESCRIPTOR_SIZE (mask);

  if (mask_kind == 1 || mask_kind == 2 || mask_kind == 4 || mask_kind == 8
#ifdef HAVE_GFC_LOGICAL_16
      || mask_kind == 16
#endif
      )
    {
      /*  Do not convert a NULL pointer as we use test for NULL below.  */
      if (mptr)
        mptr = GFOR_POINTER_TO_L1 (mptr, mask_kind);
    }
  else
    runtime_error ("Funny sized logical array");

where GFOR_POINTER_TO_L1 computes an offset between little- and big-endian
systems, and we then access the value bytewise.

If we made this more cleanly, we would add 1505 more functions to the
libgfortran library (rough count).

On the plus side, all of the pointers involved are restrict.
Comment 26 rguenther@suse.de 2019-06-06 07:23:41 UTC
On Thu, 6 Jun 2019, tkoenig at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77278
> 
> --- Comment #25 from Thomas Koenig <tkoenig at gcc dot gnu.org> ---
> (In reply to Jan Hubicka from comment #24)
> > Hi,
> > actually it won't help since C has only one bool type and not bools in
> > different sizes (why would one need that?).
> 
> "Because it's in the Fortran language standard" is probably a good
> answer as any.  For example, the standard specifies that default
> logical and default integer have to have the same size, and
> it specifies all the other logical kinds.
> 
> > I guess it would be easiest to turn Fortran frontend to use integers here.
> 
> Easiest for this case, yes.
> 
> However, this is far from the only case where integer vs. logical
> is a problem in libgfortran - to reduce code size, we've been treating
> them interchangeably for quite some time.
> 
> Really, it would be easiest if there was a way to tell the middle
> end that logical(kind=4) and integer(kind=4) could alias.

I think that's reasonably easy to do for LTO.  We'd want to keep
the default boolean_type_node size BOOLEAN_TYPEs separate but
can glob larger ones with integer types in the canonical type
merging.  We can probably do the same in non-LTO but that might
not be required.

Honza?

> There is another, possibly even worse, case.  To reduce combinatorical
> explosion of some functions, we have been using this idiom
> 
>   mask_kind = GFC_DESCRIPTOR_SIZE (mask);
> 
>   if (mask_kind == 1 || mask_kind == 2 || mask_kind == 4 || mask_kind == 8
> #ifdef HAVE_GFC_LOGICAL_16
>       || mask_kind == 16
> #endif
>       )
>     {
>       /*  Do not convert a NULL pointer as we use test for NULL below.  */
>       if (mptr)
>         mptr = GFOR_POINTER_TO_L1 (mptr, mask_kind);
>     }
>   else
>     runtime_error ("Funny sized logical array");
> 
> where GFOR_POINTER_TO_L1 computes an offset between little- and big-endian
> systems, and we then access the value bytewise.
> 
> If we made this more cleanly, we would add 1505 more functions to the
> libgfortran library (rough count).

If the "bytewise" access uses character types (or uint8_t) then
TBAA wise that should be fine.  If the pointers are restrict
you don't lose optimization either.
Comment 27 Jan Hubicka 2019-06-06 08:42:32 UTC
> 
> I think that's reasonably easy to do for LTO.  We'd want to keep
> the default boolean_type_node size BOOLEAN_TYPEs separate but
> can glob larger ones with integer types in the canonical type
> merging.  We can probably do the same in non-LTO but that might
> not be required.

Yes, we can glob other sizes of bool into integers and that should
not affect non-fortran languages where truth comes in only one size.
It would be bit inconsistent in a way that logical sized same way
as C bool will bind to C _Bool type and others will bind to integer
types of corresponding sizes.

The Fortran 2003 language draft has section on interoperability of
C and Fortran language:
https://j3-fortran.org/doc/year/10/10-007.pdf
Which says that fortran language C_BOOL should interoperate with _Bool.
Why libgfortran API functions which dispatch into C code are not
declared with C_BOOL rather than the integer sized logical?

I wrote some testcases for LTO C and fortran types interoperatibility
(gfortran.dg/lto/bind_c*)
Since my Fortran-fu is limited, they may not be complete. It would be
very useful to look into them and see if everything important is
tested and also that we have testcase for all cases we want to support
in addition to stadnard (like this one it seems) with some rationale
in them for future reference.

Honza
Comment 28 Thomas Koenig 2019-06-10 16:53:02 UTC
(In reply to Jan Hubicka from comment #27)
> > 
> > I think that's reasonably easy to do for LTO.  We'd want to keep
> > the default boolean_type_node size BOOLEAN_TYPEs separate but
> > can glob larger ones with integer types in the canonical type
> > merging.  We can probably do the same in non-LTO but that might
> > not be required.
> 
> Yes, we can glob other sizes of bool into integers and that should
> not affect non-fortran languages where truth comes in only one size.
> It would be bit inconsistent in a way that logical sized same way
> as C bool will bind to C _Bool type and others will bind to integer
> types of corresponding sizes.

That would be excellent.  It would also solve a problem with
non-standard interoperability (which people used before there
was the standard version, and which continues to be used
to this day, for example in LAPACK or BLAS).

The old code passes a standard LOGICAL to a C routine by reference,
but the C side can only use an int pointer.

> The Fortran 2003 language draft has section on interoperability of
> C and Fortran language:
> https://j3-fortran.org/doc/year/10/10-007.pdf
> Which says that fortran language C_BOOL should interoperate with _Bool.
> Why libgfortran API functions which dispatch into C code are not
> declared with C_BOOL rather than the integer sized logical?

History.  The library implementation was started long before the
standard C interface. And when we implemented the BACK intrinsic,
passing it as a C_BOOL was simply not discussed.

If we ever rewrite our ABI (again), I think it would make sense to
pass all scalar LOGICAL arguments via scalar.

> I wrote some testcases for LTO C and fortran types interoperatibility
> (gfortran.dg/lto/bind_c*)
> Since my Fortran-fu is limited, they may not be complete. It would be
> very useful to look into them and see if everything important is
> tested and also that we have testcase for all cases we want to support
> in addition to stadnard (like this one it seems) with some rationale
> in them for future reference.

One thing that should work is (according to the convention that
people use, and also to what we're using in libgfortran):

$ cat logical_f.f90
program main
  logical (kind=1) :: l1
  logical (kind=2) :: l2
  logical (kind=4) :: l4
  logical (kind=8) :: l8
  logical (kind=16) :: l16

  l1 = .true.
  l2 = .true.
  l4 = .true.
  l8 = .true.
  call logical_c (l1, l2, l4, l8, l16)
  if (l1 .or. l2 .or. l4 .or. l8 .or. l16) stop 1
end program main

subroutine foo(l1, l2, l4, l8, l16)
  logical (kind=1) :: l1
  logical (kind=2) :: l2
  logical (kind=4) :: l4
  logical (kind=8) :: l8
  logical (kind=16) :: l16

  l1 = .false.
  l2 = .false.
  l4 = .false.
  l8 = .false.
  l16 = .false.
end subroutine foo

$ cat logical_c.c
void logical_c_ (_Bool *l1, short *l2, int *l4, long *l8, long long *l16)
{
  foo_ (l1, l2, l4, l8, l16);
}

This would be a big first step in making things compatible.

Another test case which currently works, and where it would
be important that it keeps working, is

$ cat character_f.f90
program main
  character (len=10) :: a
  a = ''
  call foo(a)
  if (a /= '0123456789') stop 1
end program main

subroutine bar (a)
  character (len=*), intent(inout) :: a
  if (len(a) /= 10) stop 2
  if (a /= 'ABCDEFGHIJ') stop 3
  a = '0123456789'
end subroutine bar
$ cat character_c.c
#include <stddef.h>
#include <string.h>

void bar_ (char *a, size_t a_len);

void foo_ (char *a, size_t a_len)
{
  memcpy (a, "ABCDEFGHIJ", 10);
  bar_ (a, 10);
}

A really large positive effect would be that we could then really
tell people to use -flto to detect broken C / Fortran bindings.
Unfortunately, there are very many out there, see PR 90329.
Comment 29 Thomas Koenig 2019-06-17 16:39:03 UTC
Created attachment 46493 [details]
Patch to put all libgfortran functions into a namespace

This is something we should be doing as part of making libgfortran
LTO-compatible.  It adds all resolved library function to the libgfortran
namespace.

This does not work (i.e. causes regressions) because of functions like
pack, unpack, cshift etc where we call the same function with
different types of arguments.  We should be calling this via
gfc_array_char.  For example, here's eoshift0.c:

static void
eoshift0 (gfc_array_char * ret, const gfc_array_char * array,
	  index_type shift, const char * pbound, int which, index_type size,
	  const char *filler, index_type filler_len)

[...]
Comment 30 Jan Hubicka 2019-06-20 13:07:22 UTC
Hi,
this patch makes Fortran logicals to become C unsigned types of
corresponding size.  I think it is better than making them signed
because the globbing will affect aliasing within Fortran programs as
well.  We still ignore sign on chars and size_t, so this is still
throwing away some precision (i.e. LOGICAL of size 1 will become INTEGER
if size 1 for TBAA purposes)

Index: lto/lto-common.c
===================================================================
--- lto/lto-common.c	(revision 272506)
+++ lto/lto-common.c	(working copy)
@@ -238,6 +238,8 @@ hash_canonical_type (tree type)
      types.  */
   gcc_checking_assert (type_with_alias_set_p (type));
 
+  type = type_for_canonical_type_merging (type);
+
   /* Combine a few common features of types so that types are grouped into
      smaller sets; when searching for existing matching types to merge,
      only existing types having the same features as the new type will be
Index: testsuite/gfortran.dg/lto/bind_c-7_0.f90
===================================================================
--- testsuite/gfortran.dg/lto/bind_c-7_0.f90	(nonexistent)
+++ testsuite/gfortran.dg/lto/bind_c-7_0.f90	(working copy)
@@ -0,0 +1,33 @@
+! { dg-lto-do run }
+! { dg-lto-options {{ -O3 -flto }} }
+! this testcase tests additional interoperability of Fortran logical
+! that is not part of the standard but needed by libgfortran.
+program main
+  logical (kind=1) :: l1
+  logical (kind=2) :: l2
+  logical (kind=4) :: l4
+  logical (kind=8) :: l8
+  logical (kind=16) :: l16
+
+  l1 = .true.
+  l2 = .true.
+  l4 = .true.
+  l8 = .true.
+  call logical_c (l1, l2, l4, l8, l16)
+  if (l1 .or. l2 .or. l4 .or. l8 .or. l16) stop 1
+end program main
+
+subroutine foo(l1, l2, l4, l8, l16)
+  logical (kind=1) :: l1
+  logical (kind=2) :: l2
+  logical (kind=4) :: l4
+  logical (kind=8) :: l8
+  logical (kind=16) :: l16
+
+  l1 = .false.
+  l2 = .false.
+  l4 = .false.
+  l8 = .false.
+  l16 = .false.
+end subroutine foo
+
Index: testsuite/gfortran.dg/lto/bind_c-7_1.c
===================================================================
--- testsuite/gfortran.dg/lto/bind_c-7_1.c	(nonexistent)
+++ testsuite/gfortran.dg/lto/bind_c-7_1.c	(working copy)
@@ -0,0 +1,6 @@
+
+extern void foo_ (_Bool *l1, unsigned short *l2, unsigned int *l4, unsigned long long *l8, unsigned __int128 *l16);
+void logical_c_ (_Bool *l1, unsigned short *l2, unsigned int *l4, unsigned long long *l8, unsigned __int128 *l16)
+{
+  foo_ (l1, l2, l4, l8, l16);
+}
Index: tree.c
===================================================================
--- tree.c	(revision 272506)
+++ tree.c	(working copy)
@@ -14047,6 +14053,25 @@ type_with_interoperable_signedness (cons
 	     || TYPE_PRECISION (type) == TYPE_PRECISION (size_type_node));
 }
 
+/* Return type replacing TYPE for canonical type merging.
+   
+   We translate BOOLEAN_TYPE of size different from C _Bool to
+   unsigned integers. This makes it possible to interoperate C
+   and Fortran on Fortran's LOGICAL types which come in different
+   sizes.  This is not required by the language standard but
+   used by libgfortran.  */
+
+extern tree
+type_for_canonical_type_merging (const_tree type)
+{
+  if (TREE_CODE (type) == BOOLEAN_TYPE
+      && !tree_int_cst_equal (TYPE_SIZE (type),
+     			      TYPE_SIZE (boolean_type_node)))
+    return lang_hooks.types.type_for_mode (TYPE_MODE (type),
+		    			   true);
+  return const_cast <tree> (type);
+}
+
 /* Return true iff T1 and T2 are structurally identical for what
    TBAA is concerned.  
    This function is used both by lto.c canonical type merging and by the
@@ -14066,6 +14091,8 @@ gimple_canonical_types_compatible_p (con
       t1 = TYPE_MAIN_VARIANT (t1);
       t2 = TYPE_MAIN_VARIANT (t2);
     }
+  t1 = type_for_canonical_type_merging (t1);
+  t2 = type_for_canonical_type_merging (t2);
 
   /* Check first for the obvious case of pointer identity.  */
   if (t1 == t2)
Index: tree.h
===================================================================
--- tree.h	(revision 272506)
+++ tree.h	(working copy)
@@ -5132,9 +5141,10 @@ extern void DEBUG_FUNCTION verify_type (
 extern bool gimple_canonical_types_compatible_p (const_tree, const_tree,
 						 bool trust_type_canonical = true);
 extern bool type_with_interoperable_signedness (const_tree);
+extern tree type_for_canonical_type_merging (const_tree type);
 extern bitmap get_nonnull_args (const_tree);
 extern int get_range_pos_neg (tree);
 
 /* Return simplified tree code of type that is used for canonical type
    merging.  */
 inline enum tree_code
Comment 31 Thomas Koenig 2019-06-20 15:29:09 UTC
(
> this patch makes Fortran logicals to become C unsigned types of
> corresponding size.  I think it is better than making them signed
> because the globbing will affect aliasing within Fortran programs as
> well.

I think you're right there, I was wondering about that problem
as well.

Do I understand correctly that this type merging only happens
if LTO is enabled, and does not affect performance otherwise?

We would then have to redefine the GFC_LOGICAL_* types
in libgfortran to unsigned, but that should be doable.

Regarding the test case: As it is, this will only work if
there is a 16-byte integer. It is probably better to
use only up to 8-byte logicals, and put in a separate
test case with the 16-byte logicals and put in

! { dg-require-effective-target fortran_integer_16 }

This is looking very good, thanks!