[patch, fortran] Load scalar intent-in variables at the beginning of procedures

classic Classic list List threaded Threaded
16 messages Options
Reply | Threaded
Open this post in threaded view
|

[patch, fortran] Load scalar intent-in variables at the beginning of procedures

Thomas König
Hello world,

the attached patch loads scalar INTENT(IN) variables to a local
variable at the start of a procedure, as suggested in PR 67202, in
order to aid optimization.  This is controlled by front-end
optimization so it is easier to catch if any bugs should turn up :-)

This is done to make optimization by the middle-end easier.

I left in the parts for debugging that I added for this patch.
Seeing the difference between EXEC_INIT_ASSIGN and EXEC_ASSIGN was
particularly instructive.

Regression-tested. OK for trunk?

Regards

        Thomas

p6.diff (5K) Download Attachment
intent_optimize_3.f90 (502 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [patch, fortran] Load scalar intent-in variables at the beginning of procedures

Thomas Koenig-6
Am 11.11.19 um 22:55 schrieb Thomas König:
> Regression-tested. OK for trunk?

Of course, better with a ChangeLog entry.

2019-11-11  Thomas Koenig  <[hidden email]>

        PR fortran/67202
        * dump-parse-tree.c (debug): Add for gfc_namespace.
        (show_code_node): Add INIT_ on dumping EXEC_INIT_ASSIGN.
        * frontent-passes.c (replace_intent_in): Add prototype.  New function.
        (optimize_namespace): Call it.
        (sym_replacement): New struct.
        (replace_symbol_in_expr): New function.

2019-11-11  Thomas Koenig  <[hidden email]>

        PR fortran/67202
        * gfortran.dg/intent_optimize_3.f90: New test.
Reply | Threaded
Open this post in threaded view
|

Re: [patch, fortran] Load scalar intent-in variables at the beginning of procedures

Janne Blomqvist-3
In reply to this post by Thomas König
On Mon, Nov 11, 2019 at 11:56 PM Thomas König <[hidden email]> wrote:

>
> Hello world,
>
> the attached patch loads scalar INTENT(IN) variables to a local
> variable at the start of a procedure, as suggested in PR 67202, in
> order to aid optimization.  This is controlled by front-end
> optimization so it is easier to catch if any bugs should turn up :-)
>
> This is done to make optimization by the middle-end easier.
>
> I left in the parts for debugging that I added for this patch.
> Seeing the difference between EXEC_INIT_ASSIGN and EXEC_ASSIGN was
> particularly instructive.
>
> Regression-tested. OK for trunk?

Wouldn't it be even better to pass scalar intent(in) variables by
value? The obvious objection of course is ABI, but for procedures with
an explicit interface we're not following any particular ABI anyways?


--
Janne Blomqvist
Reply | Threaded
Open this post in threaded view
|

Re: [patch, fortran] Load scalar intent-in variables at the beginning of procedures

Thomas König
In reply to this post by Thomas König
Hi Janne,

> Ah, of course. I should have said module procedures. Or even module
> procedures without bind(C)?

It would probably be the latter.

The change would actually be rather small: If conditions are met, just add attr.value for INTENT(IN).

This is something we should probably do when we are forced into doing an ABI change by other circumstances.

Regards, Thomad
Reply | Threaded
Open this post in threaded view
|

Re: [patch, fortran] Load scalar intent-in variables at the beginning of procedures

Thomas König
In reply to this post by Thomas König
Hi Tobias,

> On 11/12/19 1:42 PM, Thomas König wrote:
>>> Ah, of course. I should have said module procedures. Or even module procedures without bind(C)?
>> It would probably be the latter. The change would actually be rather small: If conditions are met, just add attr.value for INTENT(IN). This is something we should probably do when we are forced into doing an ABI change by other circumstances.
>
> Will this still work if one does:
>
> module m
> contains
> integer function val(y)
>  integer, intent(in) :: y
>  val = 2*y
> end function val
> end module m
>
> use m
> interface
>  integer function proc(z)
>    integer, intent(in) :: z
>  end function proc
> end interface
> procedure(proc), pointer :: ff
> ff => val
> print *, ff(10)
> end

You are right, it would not work. So, scratch that idea. Maybe we should commit this as a test case so nobody gets funny ideas in two year‘s time 😉

So, I think we can then discuss the original patch.

Regards

Thomas
Reply | Threaded
Open this post in threaded view
|

Re: [patch, fortran] Load scalar intent-in variables at the beginning of procedures

Tobias Burnus-3
In reply to this post by Thomas König
Hi Thomas,

On 11/11/19 10:55 PM, Thomas König wrote:
> the attached patch loads scalar INTENT(IN) variables to a local
> variable at the start of a procedure, as suggested in PR 67202, in
> order to aid optimization.  This is controlled by front-end
> optimization so it is easier to catch if any bugs should turn up :-)

> +      if (f->sym == NULL || f->sym->attr.dimension || f->sym->attr.allocatable
> +  || f->sym->attr.optional || f->sym->attr.pointer
> +  || f->sym->attr.codimension || f->sym->attr.value
> +  || f->sym->attr.proc_pointer || f->sym->attr.target
> +  || f->sym->attr.asynchronous
> +  || f->sym->ts.type == BT_CHARACTER || f->sym->ts.type == BT_DERIVED
> +  || f->sym->ts.type == BT_CLASS)
> + continue;
I think you need to add at least VOLATILE to this list – otherwise, I
have not thought much about corner cases nor have studied the patch, sorry.

Cheers,

Tobias

Reply | Threaded
Open this post in threaded view
|

Re: [patch, fortran] Load scalar intent-in variables at the beginning of procedures

Thomas König
Hi Tobias,

> I think you need to add at least VOLATILE to this list

Yes, I'll do that.

Any other comments?

Regards

        Thomas
Reply | Threaded
Open this post in threaded view
|

Re: [patch, fortran] Load scalar intent-in variables at the beginning of procedures

Thomas Koenig-6
Hello world,

here is an update to the patch.

I have now included variables where the user did not specify INTENT(IN)
by checking that the dummy variables to be replaced by temporaries
are not, indeed, assigned a value. This also includes being passed
as an actual argument to a non-INTENT(IN) dummy argument.

Extending this led to being able to catch a few more bugs.

I have addes one test case to check where the new temporaries are added.

Regression-tested. The only change I see in the testsuite now is

XPASS: gfortran.dg/goacc/kernels-loop-n.f95   -O   scan-tree-dump-times
parloops1 "(?n)__attribute__\\(\\(oacc kernels parallelized, oacc
function \\(, , \\), oacc kernels, omp target entrypoint\\)\\)" 1

So, OK for trunk?

Regards

        Thomas

2019-11-11  Thomas Koenig  <[hidden email]>

        PR fortran/67202
        * dump-parse-tree.c (debug): Add for gfc_namespace.
        (show_code_node): Add INIT_ on dumping EXEC_INIT_ASSIGN.
        * frontent-passes.c (replace_intent_in): Add prototype.  New
        function.
        (optimize_namespace): Call it.
        (sym_replacement): New struct.
        (defined_code_callback): New function.
        (defined_expr_callback): New function.
        (replace_symbol_in_expr): New function.

2019-11-11  Thomas Koenig  <[hidden email]>

        PR fortran/67202
        * gfortran.dg/intent_optimize_3.f90: New test.
        * gfortran.dg/intent_optimize_4.f90: New test.
        * gfortran.dg/pr26246_2.f90: Add -fno-frontend-optimize to flags.



p10.diff (11K) Download Attachment
intent_optimize_4.f90 (1K) Download Attachment
intent_optimize_3.f90 (502 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [patch, fortran] Load scalar intent-in variables at the beginning of procedures

Bernhard Reutner-Fischer
On 16 November 2019 21:33:58 CET, Thomas Koenig <[hidden email]> wrote:
>Hello world,
>
>here is an update to the patch.

+  char name[GFC_MAX_SYMBOL_LEN + 1];
+  snprintf (name, GFC_MAX_SYMBOL_LEN, "__dummy_%d_%s", var_num++,
+    f->sym->name);
+
+  if (gfc_get_sym_tree (name, ns, &symtree, false) != 0)

(I) you should + sizeof(__dummy__) + 10 for unsigned long %d or the like.
(II) s/__dummy/__intent_in/ for clarity?

thanks,
Reply | Threaded
Open this post in threaded view
|

Re: [patch, fortran] Load scalar intent-in variables at the beginning of procedures

Thomas Koenig-6
Am 19.11.19 um 11:39 schrieb Bernhard Reutner-Fischer:
> +  char name[GFC_MAX_SYMBOL_LEN + 1];
> +  snprintf (name, GFC_MAX_SYMBOL_LEN, "__dummy_%d_%s", var_num++,
> +    f->sym->name);
> +
> +  if (gfc_get_sym_tree (name, ns, &symtree, false) != 0)
>
> (I) you should + sizeof(__dummy__) + 10 for unsigned long %d or the like.

GFC_MAX_SYMBOL_LEN is the maximum length of a gfortran symbol. AFAIK, it
is not possible to use a longer symbol name than that, so it needs to be
truncated. Uniqueness of the variable name is guaranteed by the var_num
variable.

If the user puts dummy arguments Supercalifragilisticexpialidociousa and
Supercalifragilisticexpialidociousb into the argument list of a
procedure, he will have to look at the numbers to differentiate them :-)

> (II) s/__dummy/__intent_in/ for clarity?

It's moved away a bit from INTENT(IN) now, because an argument which
cannot be modified (even by passing to a procedure with a dummy argument
with unknown intent) is now also handled.

Regards

        Thomas
Reply | Threaded
Open this post in threaded view
|

Re: [patch, fortran] Load scalar intent-in variables at the beginning of procedures

Janne Blomqvist-3
In reply to this post by Thomas Koenig-6
On Sat, Nov 16, 2019 at 10:34 PM Thomas Koenig <[hidden email]> wrote:

>
> Hello world,
>
> here is an update to the patch.
>
> I have now included variables where the user did not specify INTENT(IN)
> by checking that the dummy variables to be replaced by temporaries
> are not, indeed, assigned a value. This also includes being passed
> as an actual argument to a non-INTENT(IN) dummy argument.
>
> Extending this led to being able to catch a few more bugs.
>
> I have addes one test case to check where the new temporaries are added.
>
> Regression-tested. The only change I see in the testsuite now is
>
> XPASS: gfortran.dg/goacc/kernels-loop-n.f95   -O   scan-tree-dump-times
> parloops1 "(?n)__attribute__\\(\\(oacc kernels parallelized, oacc
> function \\(, , \\), oacc kernels, omp target entrypoint\\)\\)" 1

BTW, since this is done for the purpose of optimization, have you done
testing on some suitable benchmark suite such as polyhedron, whether
it a) generates any different code b) does it make it go faster?

Is there a risk of performance regressions due to higher register pressure?

--
Janne Blomqvist
Reply | Threaded
Open this post in threaded view
|

Re: [patch, fortran] Load scalar intent-in variables at the beginning of procedures

Steve Kargl
In reply to this post by Thomas Koenig-6
On Wed, Nov 20, 2019 at 10:38:30PM +0200, Janne Blomqvist wrote:

> On Wed, Nov 20, 2019 at 8:00 PM Bernhard Reutner-Fischer
> <[hidden email]> wrote:
> >
> > On 19 November 2019 23:54:55 CET, Thomas Koenig <[hidden email]> wrote:
> > >Am 19.11.19 um 11:39 schrieb Bernhard Reutner-Fischer:
> > >> +      char name[GFC_MAX_SYMBOL_LEN + 1];
> > >> +      snprintf (name, GFC_MAX_SYMBOL_LEN, "__dummy_%d_%s", var_num++,
> > >> +                f->sym->name);
> > >> +
> > >> +      if (gfc_get_sym_tree (name, ns, &symtree, false) != 0)
> > >>
> > >> (I) you should + sizeof(__dummy__) + 10 for unsigned long %d or the
> > >like.
> > >
> > >GFC_MAX_SYMBOL_LEN is the maximum length of a gfortran symbol. AFAIK,
> >
> > This is only true for user-provided names in the source code.
> >
> > Think e.g. class names as can be seen in the dumps..
>
> We have GFC_MAX_MANGLED_SYMBOL_LEN for that. *Insert my standard pet
> peeve rant that we should use heap allocated unlimited length strings
> for these rather than copying stack allocated strings around, or
> preferable a symbol table*
>

I agree with Janne.  This seems like a very good candidate
for someone that would like to contribute to gfortran, but
does not know where to start.  Any lurkers on the mailing list
care to give it shot?

--
Steve
Reply | Threaded
Open this post in threaded view
|

Re: [patch, fortran] Load scalar intent-in variables at the beginning of procedures

Thomas König
In reply to this post by Janne Blomqvist-3
Am 20.11.19 um 21:45 schrieb Janne Blomqvist:
> BTW, since this is done for the purpose of optimization, have you done
> testing on some suitable benchmark suite such as polyhedron, whether
> it a) generates any different code b) does it make it go faster?

I haven't run any actual benchmarks.

However, there is a simple example which shows its advantages.
Consider

       subroutine foo(n,m)
       m = 0
       do 100 i=1,100
         call bar
         m = m + n
  100  continue
       end

(I used old-style DO loops just because :-)

Without the optimization, the inner loop is translated to

.L2:
         xorl    %eax, %eax
         call    bar_
         movl    (%r12), %eax
         addl    %eax, 0(%rbp)
         subl    $1, %ebx
         jne     .L2

and with the optimization to

.L2:
         xorl    %eax, %eax
         call    bar_
         addl    %r12d, 0(%rbp)
         subl    $1, %ebx
         jne     .L2

so the load of the address is missing.  (Why do we zero %eax
before each call? It should not be a variadic call right?)

Of course, Fortran language rules specify that the call to bar
cannot do anything to n, but apparently we do not tell the gcc
middle end that this is the case, or maybe that there is
no way to really specify this.

(Actually, I just tried out

       subroutine foo(n,m)
       integer :: dummy_n, dummy_m
       dummy_n = n
       dummy_m = 0
       do 100 i=1,100
          call bar
          dummy_m = dummy_m + dummy_n
  100  continue
       m = dummy_m
       end

This is optimized even further:

.L2:
         xorl    %eax, %eax
         call    bar_
         subl    $1, %ebx
         jne     .L2
         imull   $100, %r12d, %r12d

So, a copy in / copy out for variables where we can not be sure that
no value is assigned?  Does anybody see a downside for that?)

> Is there a risk of performance regressions due to higher register pressure?

I don't think so. Either the compiler realizes that it can
keep the variable in a register (then it makes no difference),
or it has to load it fresh from its address (then there is
one additional register needed).

Regards

        Thomas
Reply | Threaded
Open this post in threaded view
|

Re: [patch, fortran] Load scalar intent-in variables at the beginning of procedures

Tobias Burnus
On 11/20/19 11:18 PM, Janne Blomqvist wrote:

>> Of course, Fortran language rules specify that the call to bar
>> cannot do anything to n
> Hmm, does it? What about the following modification to your testcase:

This code violates (quote from F2018):

"15.5.2.13  Restrictions on entities associated with dummy arguments"
"While an entity is associated with a dummy argument, the following
restrictions hold."
"[…] (3)   Action that affects the value of the entity or any subobject
of it shall be taken only through the dummy argument unless"
"(a) the dummy argument has the POINTER attribute, […]"
(Some more related to TARGET attribute and to coarrays, which also do
not apply here.)

Not listed there, but the asynchronous attribute (Section 8.5.4) would
be also a way to disable the optimization we are talking about.

Cheers,

Tobias

> module nmod
>    integer :: n
> end module nmod
>
> subroutine foo(n,m)
>    m = 0
>    do 100 i=1,100
>       call bar
>       m = m + n
> 100  continue
> end subroutine foo
>
> subroutine bar()
>    use nmod
>    n = 0
> end subroutine bar
>
> program main
>    use nmod
>    implicit none
>    integer :: m
>    n = 1
>    m = 0
>    call foo(n, m)
>    print *, m
> end program main
>
>
>> So, a copy in / copy out for variables where we can not be sure that
>> no value is assigned?  Does anybody see a downside for that?)
> In principle sounds good, unless my concerns above are real and affect
> this case too.
>
>>> Is there a risk of performance regressions due to higher register pressure?
>> I don't think so. Either the compiler realizes that it can
>> keep the variable in a register (then it makes no difference),
>> or it has to load it fresh from its address (then there is
>> one additional register needed).
> Yes, true. Good point.
>
>
Reply | Threaded
Open this post in threaded view
|

Re: [patch, fortran] Load scalar intent-in variables at the beginning of procedures

Tobias Burnus-3
In reply to this post by Thomas König
On 11/20/19 10:35 PM, Thomas König wrote:
>> Is there a risk of performance regressions due to higher register
>> pressure?

richi points out (on IRC) that ideally LTO IPA opts would promote the
call-by reference to call-by value – but is not sure that it indeed
happens. [In any case, Linux distros have started to compile packages
with LTO.]

One could try and see whether that indeed happens. – Still, I think the
real solution is to teach the middle end about the Fortran semantics.

Cheers,

Tobia


Reply | Threaded
Open this post in threaded view
|

Re: [patch, fortran] Load scalar intent-in variables at the beginning of procedures

Tobias Burnus-3
Hi Richard,

On 11/21/19 1:16 PM, Richard Biener wrote:
> OK, so I found it, it's handled via SSA_NAME_POINTS_TO_READONLY_MEMORY
> which is initialized during the rewrite into SSA form from the
> information given by the "fn spec" attribute […] so when the frontend
> sets "fn spec" from the intent it should already work.

Which I can confirm for the following made-up example:

real function foo(n)
   implicit none (type)
   integer, intent(in) :: n
   integer :: i
   foo = 0.5
   if (n /= 0) return
   call bar()
   do i = 1, n
     foo = foo + sin(foo)
   end do
end

The optimized dump shows the following, i.e. GCC nicely optimizes both the loop and the 'sin' call away:

foo (integer(kind=4) & restrict n)
{
   integer(kind=4) _1;
   <bb 2> [local count: 241635843]:
   _1 = *n_9(D);
   if (_1 != 0)
     goto <bb 4>; [51.12%]
   else
     goto <bb 3>; [48.88%]

   <bb 3> [local count: 118111600]:
   bar ();

   <bb 4> [local count: 241635844]:
   return 5.0e-1;
}

I think this optimization permits some crucial optimizations.
I have not fully followed the later versions of the patch whether
they exploit some additional language semantics to permit optimizations
even without intent(in), but the initial discussion started with intent(in).

> But the examples I saw above didn't use INTENT(IN) for the scalar
> parameters.

I concur that a well-written program should make use of intent(in) where
sensible.

There are cases, which could be optimized likewise – but based on the
case that the pointer address cannot escape instead of just assuming
that the argument cannot change. – The question is how to convey this to
the middle end.

I wonder whether there is a 'fn attr' which can tell that the first
argument of 'print_i' does not cause the address of 'i' escape. If so,
one could mark all procedure arguments such – unless they have the
pointer, target or asynchronous attribute or are coarrays. [Probably
needs some fine tuning.]

In this example, variable values do change, but only in a controlled way
('print_i' could change it, 'bar' cannot). The semantic is also mainly a
property of the (local) variable (or dummy argument) declaration and not
of the functions which are called – although, if 'i' has no target
attribute, print_i's argument cannot have neither – hence, one could
generated a function interface

real function foo(i, y)
   implicit none (type)
   integer :: i
   real :: y
   foo = 0.0
   call print_i(i)
   i = 5
   call bar()  ! < this prevents optimizing the sin(y) away
   if (i == 5) return
   foo = sin(y)
end

Fortran semantics implies that 'i' can only change after the 'i = 5' if:
'i' has the TARGET (or POINTER) attribute. Or it is possible if 'i' has
the ASYNCHRONOUS or VOLATILE attribute – or it is a coarray (where a
remote image can modify the local value).

For asynchronous, it would be something like "call read_i(i); call
wait()" which is structurally the same as above.

TARGET: "An object without the TARGET attribute shall not have a pointer
associated with it."
VOLATILE: "may be referenced, defined, or become undefined, by
means20not specified by the program"
ASYNCHRONOUS: "An entity with the ASYNCHRONOUS attribute is a variable,
and may be subject to asynchronous input/output or asynchronous
communication."

Tobias