Discussion:
Optimization of add_dt_to_dt_list() in resolve.c
Andrew Benson
2018-05-24 22:53:29 UTC
Permalink
I've been attempting to track down some of the causes of very long compile
times for files which use modules that contain a large number of symbols. The
worst case offender in my code takes around 12 minutes to compile.

After profiling f951 for this source file it turns out that the majority of the
time is spent in add_dt_to_dt_list() in resolve.c. In cases where the number
of symbols imported becomes very large (~10,000 in some cases in this code),
the O(N) search in this function becomes inefficient.

A simple optimization for this problem seems to be to just have the gfc_symbol
struct include a pointer back to the corresponding entry in the
gfc_derived_types list. It's then fast to check if a symbol is already on that
list by checking if this pointer is non-null. (It could just as easily be an
int in gfc_symbol which indicates if the symbol is already added to the list -
I don't know if having a pointer to the list entry is useful for any other
reason.)

With this change in place compile times are much faster - my worst case
offender now takes just under 1 minute to compile.

My patch is attached. I freely admit that I have only a very shallow
understanding of the inner workings of the compiler, so I would not be
surprised if there are good reasons not to do this. I did "make check-fortran"
and did not see any failures. If any one wants to try this out and/or provide
any feedback I'd be happy to hear it.

Thanks,
Andrew
--
* Andrew Benson: http://users.obs.carnegiescience.edu/abenson/contact.html

* Galacticus: https://bitbucket.org/abensonca/galacticus
Richard Biener
2018-05-25 07:06:22 UTC
Permalink
Post by Andrew Benson
I've been attempting to track down some of the causes of very long compile
times for files which use modules that contain a large number of symbols. The
worst case offender in my code takes around 12 minutes to compile.
After profiling f951 for this source file it turns out that the majority of the
time is spent in add_dt_to_dt_list() in resolve.c. In cases where the number
of symbols imported becomes very large (~10,000 in some cases in this code),
the O(N) search in this function becomes inefficient.
A simple optimization for this problem seems to be to just have the gfc_symbol
struct include a pointer back to the corresponding entry in the
gfc_derived_types list. It's then fast to check if a symbol is already on that
list by checking if this pointer is non-null. (It could just as easily be an
int in gfc_symbol which indicates if the symbol is already added to the list -
I don't know if having a pointer to the list entry is useful for any other
reason.)
With this change in place compile times are much faster - my worst case
offender now takes just under 1 minute to compile.
My patch is attached. I freely admit that I have only a very shallow
understanding of the inner workings of the compiler, so I would not be
surprised if there are good reasons not to do this. I did "make check-fortran"
and did not see any failures. If any one wants to try this out and/or provide
any feedback I'd be happy to hear it.
It looks like it would be cheaper to simply embed gtc_dt_list *next in
gfc_symbol?
(in case a gfc_symbol can be only on one gfc_dt_list which your patch
assumes as well)

Richard.
Post by Andrew Benson
Thanks,
Andrew
--
* Andrew Benson: http://users.obs.carnegiescience.edu/abenson/contact.html
* Galacticus: https://bitbucket.org/abensonca/galacticus
Richard Biener
2018-05-25 07:13:03 UTC
Permalink
On Fri, May 25, 2018 at 12:53 AM Andrew Benson <
Post by Andrew Benson
I've been attempting to track down some of the causes of very long compile
times for files which use modules that contain a large number of
symbols.
The
Post by Andrew Benson
worst case offender in my code takes around 12 minutes to compile.
After profiling f951 for this source file it turns out that the majority
of the
Post by Andrew Benson
time is spent in add_dt_to_dt_list() in resolve.c. In cases where the
number
Post by Andrew Benson
of symbols imported becomes very large (~10,000 in some cases in this
code),
Post by Andrew Benson
the O(N) search in this function becomes inefficient.
A simple optimization for this problem seems to be to just have the
gfc_symbol
Post by Andrew Benson
struct include a pointer back to the corresponding entry in the
gfc_derived_types list. It's then fast to check if a symbol is already
on
that
Post by Andrew Benson
list by checking if this pointer is non-null. (It could just as easily
be
an
Post by Andrew Benson
int in gfc_symbol which indicates if the symbol is already added to the
list -
Post by Andrew Benson
I don't know if having a pointer to the list entry is useful for any other
reason.)
With this change in place compile times are much faster - my worst case
offender now takes just under 1 minute to compile.
My patch is attached. I freely admit that I have only a very shallow
understanding of the inner workings of the compiler, so I would not be
surprised if there are good reasons not to do this. I did "make
check-fortran"
Post by Andrew Benson
and did not see any failures. If any one wants to try this out and/or
provide
Post by Andrew Benson
any feedback I'd be happy to hear it.
It looks like it would be cheaper to simply embed gtc_dt_list *next in
gfc_symbol?
(in case a gfc_symbol can be only on one gfc_dt_list which your patch
assumes as well)
Note you'll need to make the list cyclic to make the member test work
as sym->next_dt != NULL.

Also note that gfc_get_derived_type has another linear list walk so it
looks like derived type processing is still quadratic in the number of
derived types after the change. That suggests that eventually
making the derived_types list a hash-map of gfc_symbol to
the common derived type (symbol) would be a better fix, thus doing
the merging at dt-"list" insertion time.

Of course that's likely a bigger change that needs more thorough
understanding
of how derived types work. A fix along your idea looks simple enough
and has obvious benefits as you show so it might be even appropriate for
release branches.

Richard.
Richard.
Post by Andrew Benson
Thanks,
Andrew
--
http://users.obs.carnegiescience.edu/abenson/contact.html
Post by Andrew Benson
* Galacticus: https://bitbucket.org/abensonca/galacticus
Andrew Benson
2018-05-25 21:54:49 UTC
Permalink
Richard:

Thanks for the suggestion. I changed my patch (new version attached) so that
there's a *dt_next in gfc_symbol, which is now used to construct the
(circular) linked list. There were a couple places where I had to change the
order in which clean up of symbols and derived type lists were done - it's now
necessary to free the derived type list before its associated symbols (since
the symbols carry the links for the derived type list).

This passes "make check-fortran" and seems to retain the speed-up from my
original patch.

Thanks,
Andrew
Post by Andrew Benson
Post by Andrew Benson
I've been attempting to track down some of the causes of very long compile
times for files which use modules that contain a large number of symbols.
The
Post by Andrew Benson
worst case offender in my code takes around 12 minutes to compile.
After profiling f951 for this source file it turns out that the majority
of the
Post by Andrew Benson
time is spent in add_dt_to_dt_list() in resolve.c. In cases where the
number
Post by Andrew Benson
of symbols imported becomes very large (~10,000 in some cases in this
code),
Post by Andrew Benson
the O(N) search in this function becomes inefficient.
A simple optimization for this problem seems to be to just have the
gfc_symbol
Post by Andrew Benson
struct include a pointer back to the corresponding entry in the
gfc_derived_types list. It's then fast to check if a symbol is already on
that
Post by Andrew Benson
list by checking if this pointer is non-null. (It could just as easily be
an
Post by Andrew Benson
int in gfc_symbol which indicates if the symbol is already added to the
list -
Post by Andrew Benson
I don't know if having a pointer to the list entry is useful for any other
reason.)
With this change in place compile times are much faster - my worst case
offender now takes just under 1 minute to compile.
My patch is attached. I freely admit that I have only a very shallow
understanding of the inner workings of the compiler, so I would not be
surprised if there are good reasons not to do this. I did "make
check-fortran"
Post by Andrew Benson
and did not see any failures. If any one wants to try this out and/or
provide
Post by Andrew Benson
any feedback I'd be happy to hear it.
It looks like it would be cheaper to simply embed gtc_dt_list *next in
gfc_symbol?
(in case a gfc_symbol can be only on one gfc_dt_list which your patch
assumes as well)
Richard.
Post by Andrew Benson
Thanks,
Andrew
--
* Andrew Benson: http://users.obs.carnegiescience.edu/abenson/contact.html
* Galacticus: https://bitbucket.org/abensonca/galacticus
--
* Andrew Benson: http://users.obs.carnegiescience.edu/abenson/contact.html

* Galacticus: https://bitbucket.org/abensonca/galacticus
Richard Biener
2018-05-28 09:54:41 UTC
Permalink
Post by Andrew Benson
Thanks for the suggestion. I changed my patch (new version attached) so that
there's a *dt_next in gfc_symbol, which is now used to construct the
(circular) linked list. There were a couple places where I had to change the
order in which clean up of symbols and derived type lists were done - it's now
necessary to free the derived type list before its associated symbols (since
the symbols carry the links for the derived type list).
Hmm, it still has the indirection via gfc_dt_list. I think it should be
possible
to do away with gfc_dt_list objects alltogether by no doing
sym->dt_next->derived
but sym->derived thus

@@ -1611,6 +1611,9 @@ typedef struct gfc_symbol

/* Link to corresponding association-list if this is an associate name.
*/
struct gfc_association_list *assoc;
+
+ /* Link to next entry in derived type list */
+ gfc_symbol *dt_next;
}
gfc_symbol;

that means for example gfc_free_dt_list can be simply removed. The
gfc_derived_types global would then point to the first derived type
directly.

Richard.
Post by Andrew Benson
This passes "make check-fortran" and seems to retain the speed-up from my
original patch.
Thanks,
Andrew
On Fri, May 25, 2018 at 12:53 AM Andrew Benson <
Post by Andrew Benson
I've been attempting to track down some of the causes of very long compile
times for files which use modules that contain a large number of symbols.
The
Post by Andrew Benson
worst case offender in my code takes around 12 minutes to compile.
After profiling f951 for this source file it turns out that the majority
of the
Post by Andrew Benson
time is spent in add_dt_to_dt_list() in resolve.c. In cases where the
number
Post by Andrew Benson
of symbols imported becomes very large (~10,000 in some cases in this
code),
Post by Andrew Benson
the O(N) search in this function becomes inefficient.
A simple optimization for this problem seems to be to just have the
gfc_symbol
Post by Andrew Benson
struct include a pointer back to the corresponding entry in the
gfc_derived_types list. It's then fast to check if a symbol is already on
that
Post by Andrew Benson
list by checking if this pointer is non-null. (It could just as easily be
an
Post by Andrew Benson
int in gfc_symbol which indicates if the symbol is already added to the
list -
Post by Andrew Benson
I don't know if having a pointer to the list entry is useful for any other
reason.)
With this change in place compile times are much faster - my worst case
offender now takes just under 1 minute to compile.
My patch is attached. I freely admit that I have only a very shallow
understanding of the inner workings of the compiler, so I would not be
surprised if there are good reasons not to do this. I did "make
check-fortran"
Post by Andrew Benson
and did not see any failures. If any one wants to try this out and/or
provide
Post by Andrew Benson
any feedback I'd be happy to hear it.
It looks like it would be cheaper to simply embed gtc_dt_list *next in
gfc_symbol?
(in case a gfc_symbol can be only on one gfc_dt_list which your patch
assumes as well)
Richard.
Post by Andrew Benson
Thanks,
Andrew
--
http://users.obs.carnegiescience.edu/abenson/contact.html
Post by Andrew Benson
Post by Andrew Benson
* Galacticus: https://bitbucket.org/abensonca/galacticus
--
* Andrew Benson: http://users.obs.carnegiescience.edu/abenson/contact.html
* Galacticus: https://bitbucket.org/abensonca/galacticus
Andrew Benson
2018-05-29 20:24:05 UTC
Permalink
Yes - definitely possible to remove gfc_dt_list entirely - new patch is
attached.

Thanks,
Andrew
Post by Andrew Benson
Post by Andrew Benson
Thanks for the suggestion. I changed my patch (new version attached) so
that
Post by Andrew Benson
there's a *dt_next in gfc_symbol, which is now used to construct the
(circular) linked list. There were a couple places where I had to change
the
Post by Andrew Benson
order in which clean up of symbols and derived type lists were done -
it's now
Post by Andrew Benson
necessary to free the derived type list before its associated symbols
(since
Post by Andrew Benson
the symbols carry the links for the derived type list).
Hmm, it still has the indirection via gfc_dt_list. I think it should be
possible
to do away with gfc_dt_list objects alltogether by no doing
sym->dt_next->derived
but sym->derived thus
@@ -1611,6 +1611,9 @@ typedef struct gfc_symbol
/* Link to corresponding association-list if this is an associate name.
*/
struct gfc_association_list *assoc;
+
+ /* Link to next entry in derived type list */
+ gfc_symbol *dt_next;
}
gfc_symbol;
that means for example gfc_free_dt_list can be simply removed. The
gfc_derived_types global would then point to the first derived type
directly.
Richard.
Post by Andrew Benson
This passes "make check-fortran" and seems to retain the speed-up from my
original patch.
Thanks,
Andrew
On Fri, May 25, 2018 at 12:53 AM Andrew Benson <
Post by Andrew Benson
I've been attempting to track down some of the causes of very long
compile
Post by Andrew Benson
Post by Andrew Benson
times for files which use modules that contain a large number of
symbols.
Post by Andrew Benson
The
Post by Andrew Benson
worst case offender in my code takes around 12 minutes to compile.
After profiling f951 for this source file it turns out that the
majority
Post by Andrew Benson
of the
Post by Andrew Benson
time is spent in add_dt_to_dt_list() in resolve.c. In cases where the
number
Post by Andrew Benson
of symbols imported becomes very large (~10,000 in some cases in this
code),
Post by Andrew Benson
the O(N) search in this function becomes inefficient.
A simple optimization for this problem seems to be to just have the
gfc_symbol
Post by Andrew Benson
struct include a pointer back to the corresponding entry in the
gfc_derived_types list. It's then fast to check if a symbol is
already on
Post by Andrew Benson
that
Post by Andrew Benson
list by checking if this pointer is non-null. (It could just as
easily be
Post by Andrew Benson
an
Post by Andrew Benson
int in gfc_symbol which indicates if the symbol is already added to
the
Post by Andrew Benson
list -
Post by Andrew Benson
I don't know if having a pointer to the list entry is useful for any
other
Post by Andrew Benson
Post by Andrew Benson
reason.)
With this change in place compile times are much faster - my worst
case
Post by Andrew Benson
Post by Andrew Benson
offender now takes just under 1 minute to compile.
My patch is attached. I freely admit that I have only a very shallow
understanding of the inner workings of the compiler, so I would not be
surprised if there are good reasons not to do this. I did "make
check-fortran"
Post by Andrew Benson
and did not see any failures. If any one wants to try this out and/or
provide
Post by Andrew Benson
any feedback I'd be happy to hear it.
It looks like it would be cheaper to simply embed gtc_dt_list *next in
gfc_symbol?
(in case a gfc_symbol can be only on one gfc_dt_list which your patch
assumes as well)
Richard.
Post by Andrew Benson
Thanks,
Andrew
--
http://users.obs.carnegiescience.edu/abenson/contact.html
Post by Andrew Benson
Post by Andrew Benson
* Galacticus: https://bitbucket.org/abensonca/galacticus
--
* Andrew Benson: http://users.obs.carnegiescience.edu/abenson/contact.html
* Galacticus: https://bitbucket.org/abensonca/galacticus
--
* Andrew Benson: http://users.obs.carnegiescience.edu/abenson/contact.html

* Galacticus: https://bitbucket.org/abensonca/galacticus
Richard Biener
2018-05-30 09:43:58 UTC
Permalink
Post by Andrew Benson
Yes - definitely possible to remove gfc_dt_list entirely - new patch is
attached.
This looks good to me but it still requires review/ack from fortran people.

Note another trick commonly used for cyclic lists is to make the "head"
always present which could be done by making gfc_derived_types a
gfc_symbol (non-pointer), chaining to itself. Then you can elide
the if (gfc_derived_types) checks and iterate via

for (gfc_sybol *sym = gfc_derived_types.dt_next; sym != &gfc_derived_types;
sym = sym->dt_next)
...

similar appending and removing lose some special cases.

Having a global gfc_symbol with just one pointer in it used may seem a
litte ugly though
(and you have to init it somewhere suitable unless you want to write up a
huge mostly
zero static initializer).

Richard.
Post by Andrew Benson
Thanks,
Andrew
On Fri, May 25, 2018 at 11:54 PM Andrew Benson <
Post by Andrew Benson
Thanks for the suggestion. I changed my patch (new version attached) so
that
Post by Andrew Benson
there's a *dt_next in gfc_symbol, which is now used to construct the
(circular) linked list. There were a couple places where I had to change
the
Post by Andrew Benson
order in which clean up of symbols and derived type lists were done -
it's now
Post by Andrew Benson
necessary to free the derived type list before its associated symbols
(since
Post by Andrew Benson
the symbols carry the links for the derived type list).
Hmm, it still has the indirection via gfc_dt_list. I think it should be
possible
to do away with gfc_dt_list objects alltogether by no doing
sym->dt_next->derived
but sym->derived thus
@@ -1611,6 +1611,9 @@ typedef struct gfc_symbol
/* Link to corresponding association-list if this is an associate name.
*/
struct gfc_association_list *assoc;
+
+ /* Link to next entry in derived type list */
+ gfc_symbol *dt_next;
}
gfc_symbol;
that means for example gfc_free_dt_list can be simply removed. The
gfc_derived_types global would then point to the first derived type
directly.
Richard.
Post by Andrew Benson
This passes "make check-fortran" and seems to retain the speed-up from my
original patch.
Thanks,
Andrew
On Fri, May 25, 2018 at 12:53 AM Andrew Benson <
Post by Andrew Benson
I've been attempting to track down some of the causes of very long
compile
Post by Andrew Benson
Post by Andrew Benson
times for files which use modules that contain a large number of
symbols.
Post by Andrew Benson
The
Post by Andrew Benson
worst case offender in my code takes around 12 minutes to compile.
After profiling f951 for this source file it turns out that the
majority
Post by Andrew Benson
of the
Post by Andrew Benson
time is spent in add_dt_to_dt_list() in resolve.c. In cases where the
number
Post by Andrew Benson
of symbols imported becomes very large (~10,000 in some cases in this
code),
Post by Andrew Benson
the O(N) search in this function becomes inefficient.
A simple optimization for this problem seems to be to just have the
gfc_symbol
Post by Andrew Benson
struct include a pointer back to the corresponding entry in the
gfc_derived_types list. It's then fast to check if a symbol is
already on
Post by Andrew Benson
that
Post by Andrew Benson
list by checking if this pointer is non-null. (It could just as
easily be
Post by Andrew Benson
an
Post by Andrew Benson
int in gfc_symbol which indicates if the symbol is already added to
the
Post by Andrew Benson
list -
Post by Andrew Benson
I don't know if having a pointer to the list entry is useful for any
other
Post by Andrew Benson
Post by Andrew Benson
reason.)
With this change in place compile times are much faster - my worst
case
Post by Andrew Benson
Post by Andrew Benson
offender now takes just under 1 minute to compile.
My patch is attached. I freely admit that I have only a very shallow
understanding of the inner workings of the compiler, so I would not be
surprised if there are good reasons not to do this. I did "make
check-fortran"
Post by Andrew Benson
and did not see any failures. If any one wants to try this out and/or
provide
Post by Andrew Benson
any feedback I'd be happy to hear it.
It looks like it would be cheaper to simply embed gtc_dt_list *next in
gfc_symbol?
(in case a gfc_symbol can be only on one gfc_dt_list which your patch
assumes as well)
Richard.
Post by Andrew Benson
Thanks,
Andrew
--
http://users.obs.carnegiescience.edu/abenson/contact.html
Post by Andrew Benson
Post by Andrew Benson
* Galacticus: https://bitbucket.org/abensonca/galacticus
--
http://users.obs.carnegiescience.edu/abenson/contact.html
Post by Andrew Benson
Post by Andrew Benson
* Galacticus: https://bitbucket.org/abensonca/galacticus
--
* Andrew Benson: http://users.obs.carnegiescience.edu/abenson/contact.html
* Galacticus: https://bitbucket.org/abensonca/galacticus
Andrew Benson
2018-05-30 17:01:12 UTC
Permalink
Post by Richard Biener
Post by Andrew Benson
Yes - definitely possible to remove gfc_dt_list entirely - new patch is
attached.
This looks good to me but it still requires review/ack from fortran people.
Thanks - I'll wait on input from the fortran devs.
Post by Richard Biener
Note another trick commonly used for cyclic lists is to make the "head"
always present which could be done by making gfc_derived_types a
gfc_symbol (non-pointer), chaining to itself. Then you can elide
the if (gfc_derived_types) checks and iterate via
for (gfc_sybol *sym = gfc_derived_types.dt_next; sym != &gfc_derived_types;
sym = sym->dt_next)
...
similar appending and removing lose some special cases.
Having a global gfc_symbol with just one pointer in it used may seem a
litte ugly though
(and you have to init it somewhere suitable unless you want to write up a
huge mostly
zero static initializer).
That would definitely make the code cleaner. I'll wait on input from the
fortran devs to see if any of them have an opinion on this - and I'd probably
need some guidance on where to do the init.

-Andrew
--
* Andrew Benson: http://users.obs.carnegiescience.edu/abenson/contact.html

* Galacticus: https://bitbucket.org/abensonca/galacticus
Steve Kargl
2018-05-30 18:25:41 UTC
Permalink
Post by Andrew Benson
Post by Richard Biener
Post by Andrew Benson
Yes - definitely possible to remove gfc_dt_list entirely - new patch is
attached.
This looks good to me but it still requires review/ack from fortran people.
Thanks - I'll wait on input from the fortran devs.
Andrew,

You and Richard were having what appeared to be a productive
exchange, so I thought I would sit on the side and let it
reach a conclusion. I'll take a look at what you've done
this weekend. Hopefully, others can also cast an eye over
the patch as well.

PS: Welcome to the wonderful wonder of gfortran hacking.
--
Steve
Andrew Benson
2018-05-30 18:37:44 UTC
Permalink
Post by Steve Kargl
Post by Andrew Benson
On Tue, May 29, 2018 at 10:24 PM Andrew Benson
Post by Andrew Benson
Yes - definitely possible to remove gfc_dt_list entirely - new patch is
attached.
This looks good to me but it still requires review/ack from fortran people.
Thanks - I'll wait on input from the fortran devs.
You and Richard were having what appeared to be a productive
exchange, so I thought I would sit on the side and let it
reach a conclusion. I'll take a look at what you've done
this weekend. Hopefully, others can also cast an eye over
the patch as well.
Thanks - greatly appreciated!
Post by Steve Kargl
PS: Welcome to the wonderful wonder of gfortran hacking.
I suspect this is a slippery slope.... but so far it's been fun and
instructive for me at least!

-Andrew
--
* Andrew Benson: http://users.obs.carnegiescience.edu/abenson/contact.html

* Galacticus: https://bitbucket.org/abensonca/galacticus
Janus Weil
2018-05-30 20:42:46 UTC
Permalink
Hi Andrew,
Post by Andrew Benson
Yes - definitely possible to remove gfc_dt_list entirely - new patch is
attached.
since you already got some 'contentual' comments, I'll give you some
more 'formal' ones ...


+ if (!derived->dt_next) {
+ if (gfc_derived_types) {
+ derived->dt_next = gfc_derived_types->dt_next;
+ gfc_derived_types->dt_next = derived;
+ } else {
+ derived->dt_next = derived;
+ }
+ gfc_derived_types = derived;
+ }

Here and in some other hunks you're not conforming with the GNU coding
style, which demands that opening and closing braces should be on
separate lines (and at a new indentation level).


- for (dt = gfc_derived_types; dt; dt = dt->next)
- gfc_copy_dt_decls_ifequal (derived, dt->derived, false);
-
+ if (gfc_derived_types) {
+ dt = gfc_derived_types;
+ for (;;)
+ {
+ gfc_copy_dt_decls_ifequal (derived, dt, false);
+ if (dt->dt_next == gfc_derived_types)
+ break;
+ dt = dt->dt_next;
+ }
+ }

Is there a particular reason why you changed the loop to "for (;;)" ?
I find the old style a bit clearer and more compact. Also I think it's
more common in gfortran.

Btw, do you already have a copyright assignment on file? If not,
you'll probably need one (see https://gcc.gnu.org/contribute.html).

Thanks for your contribution and welcome to the gfortran team :)

Cheers,
Janus
Post by Andrew Benson
Post by Andrew Benson
Post by Andrew Benson
Thanks for the suggestion. I changed my patch (new version attached) so
that
Post by Andrew Benson
there's a *dt_next in gfc_symbol, which is now used to construct the
(circular) linked list. There were a couple places where I had to change
the
Post by Andrew Benson
order in which clean up of symbols and derived type lists were done -
it's now
Post by Andrew Benson
necessary to free the derived type list before its associated symbols
(since
Post by Andrew Benson
the symbols carry the links for the derived type list).
Hmm, it still has the indirection via gfc_dt_list. I think it should be
possible
to do away with gfc_dt_list objects alltogether by no doing
sym->dt_next->derived
but sym->derived thus
@@ -1611,6 +1611,9 @@ typedef struct gfc_symbol
/* Link to corresponding association-list if this is an associate name.
*/
struct gfc_association_list *assoc;
+
+ /* Link to next entry in derived type list */
+ gfc_symbol *dt_next;
}
gfc_symbol;
that means for example gfc_free_dt_list can be simply removed. The
gfc_derived_types global would then point to the first derived type
directly.
Richard.
Post by Andrew Benson
This passes "make check-fortran" and seems to retain the speed-up from my
original patch.
Thanks,
Andrew
On Fri, May 25, 2018 at 12:53 AM Andrew Benson <
Post by Andrew Benson
I've been attempting to track down some of the causes of very long
compile
Post by Andrew Benson
Post by Andrew Benson
times for files which use modules that contain a large number of
symbols.
Post by Andrew Benson
The
Post by Andrew Benson
worst case offender in my code takes around 12 minutes to compile.
After profiling f951 for this source file it turns out that the
majority
Post by Andrew Benson
of the
Post by Andrew Benson
time is spent in add_dt_to_dt_list() in resolve.c. In cases where the
number
Post by Andrew Benson
of symbols imported becomes very large (~10,000 in some cases in this
code),
Post by Andrew Benson
the O(N) search in this function becomes inefficient.
A simple optimization for this problem seems to be to just have the
gfc_symbol
Post by Andrew Benson
struct include a pointer back to the corresponding entry in the
gfc_derived_types list. It's then fast to check if a symbol is
already on
Post by Andrew Benson
that
Post by Andrew Benson
list by checking if this pointer is non-null. (It could just as
easily be
Post by Andrew Benson
an
Post by Andrew Benson
int in gfc_symbol which indicates if the symbol is already added to
the
Post by Andrew Benson
list -
Post by Andrew Benson
I don't know if having a pointer to the list entry is useful for any
other
Post by Andrew Benson
Post by Andrew Benson
reason.)
With this change in place compile times are much faster - my worst
case
Post by Andrew Benson
Post by Andrew Benson
offender now takes just under 1 minute to compile.
My patch is attached. I freely admit that I have only a very shallow
understanding of the inner workings of the compiler, so I would not be
surprised if there are good reasons not to do this. I did "make
check-fortran"
Post by Andrew Benson
and did not see any failures. If any one wants to try this out and/or
provide
Post by Andrew Benson
any feedback I'd be happy to hear it.
It looks like it would be cheaper to simply embed gtc_dt_list *next in
gfc_symbol?
(in case a gfc_symbol can be only on one gfc_dt_list which your patch
assumes as well)
Richard.
Post by Andrew Benson
Thanks,
Andrew
--
http://users.obs.carnegiescience.edu/abenson/contact.html
Post by Andrew Benson
Post by Andrew Benson
* Galacticus: https://bitbucket.org/abensonca/galacticus
--
* Andrew Benson: http://users.obs.carnegiescience.edu/abenson/contact.html
* Galacticus: https://bitbucket.org/abensonca/galacticus
--
* Andrew Benson: http://users.obs.carnegiescience.edu/abenson/contact.html
* Galacticus: https://bitbucket.org/abensonca/galacticus
Andrew Benson
2018-05-30 22:22:28 UTC
Permalink
Hi Janus,
Post by Janus Weil
Hi Andrew,
Post by Andrew Benson
Yes - definitely possible to remove gfc_dt_list entirely - new patch is
attached.
since you already got some 'contentual' comments, I'll give you some
more 'formal' ones ...
+ if (!derived->dt_next) {
+ if (gfc_derived_types) {
+ derived->dt_next = gfc_derived_types->dt_next;
+ gfc_derived_types->dt_next = derived;
+ } else {
+ derived->dt_next = derived;
+ }
+ gfc_derived_types = derived;
+ }
Here and in some other hunks you're not conforming with the GNU coding
style, which demands that opening and closing braces should be on
separate lines (and at a new indentation level).
Thanks - I'll fix those cases.
Post by Janus Weil
- for (dt = gfc_derived_types; dt; dt = dt->next)
- gfc_copy_dt_decls_ifequal (derived, dt->derived, false);
-
+ if (gfc_derived_types) {
+ dt = gfc_derived_types;
+ for (;;)
+ {
+ gfc_copy_dt_decls_ifequal (derived, dt, false);
+ if (dt->dt_next == gfc_derived_types)
+ break;
+ dt = dt->dt_next;
+ }
+ }
Is there a particular reason why you changed the loop to "for (;;)" ?
I find the old style a bit clearer and more compact. Also I think it's
more common in gfortran.
I changed the original for loop since it wasn't possible (as far as I could
see) to have an exit condition that worked now that list is circularly linked
(i.e. "dt" never becomes NULL, and testing for dt->dt_next ==
gfc_derived_types doesn't work as it would miss the final entry in the list).
So, I used for(;;) and added the exit condition explicitly into the loop.

But, I agree, it's not very clear. An alternative would be something such as:

- for (dt = gfc_derived_types; dt; dt = dt->next)
- gfc_copy_dt_decls_ifequal (derived, dt->derived, false);
-
+ for (dt = gfc_derived_types; dt; dt = dt->dt_next)
+ {
+ gfc_copy_dt_decls_ifequal (derived, dt, false);
+ if (dt->dt_next == gfc_derived_types)
+ break;
+ }
+

which is more compact, and has the advantage that if doesn't require an "if
(gfc_derived_types)". Do you think that's a better approach?
Post by Janus Weil
Btw, do you already have a copyright assignment on file? If not,
you'll probably need one (see https://gcc.gnu.org/contribute.html).
I don't, so will need to get that figured out.
Post by Janus Weil
Thanks for your contribution and welcome to the gfortran team :)
Thanks!

-Andrew
--
* Andrew Benson: http://users.obs.carnegiescience.edu/abenson/contact.html

* Galacticus: https://bitbucket.org/abensonca/galacticus
Janus Weil
2018-05-31 08:56:47 UTC
Permalink
Post by Andrew Benson
Post by Janus Weil
- for (dt = gfc_derived_types; dt; dt = dt->next)
- gfc_copy_dt_decls_ifequal (derived, dt->derived, false);
-
+ if (gfc_derived_types) {
+ dt = gfc_derived_types;
+ for (;;)
+ {
+ gfc_copy_dt_decls_ifequal (derived, dt, false);
+ if (dt->dt_next == gfc_derived_types)
+ break;
+ dt = dt->dt_next;
+ }
+ }
Is there a particular reason why you changed the loop to "for (;;)" ?
I find the old style a bit clearer and more compact. Also I think it's
more common in gfortran.
I changed the original for loop since it wasn't possible (as far as I could
see) to have an exit condition that worked now that list is circularly linked
(i.e. "dt" never becomes NULL, and testing for dt->dt_next ==
gfc_derived_types doesn't work as it would miss the final entry in the list).
So, I used for(;;) and added the exit condition explicitly into the loop.
- for (dt = gfc_derived_types; dt; dt = dt->next)
- gfc_copy_dt_decls_ifequal (derived, dt->derived, false);
-
+ for (dt = gfc_derived_types; dt; dt = dt->dt_next)
+ {
+ gfc_copy_dt_decls_ifequal (derived, dt, false);
+ if (dt->dt_next == gfc_derived_types)
+ break;
+ }
+
which is more compact, and has the advantage that if doesn't require an "if
(gfc_derived_types)". Do you think that's a better approach?
Yes, definitely looks better to me. Note that there is another such
case further up in gfc_get_derived_type. Actually I'd also move the
declaration of dt ("gfc_symbol *dt") into the loops, in order to make
it more local (it's not used outside).

Btw, another thing that you'll need is a ChangeLog entry that lists
every file and function touched by your patch and gives a short
description of the modifications. You'll find plenty of examples for
this in gcc/fortran/ChangeLog.

Cheers,
Janus
Andrew Benson
2018-05-31 18:04:04 UTC
Permalink
Post by Janus Weil
Post by Andrew Benson
Post by Janus Weil
- for (dt = gfc_derived_types; dt; dt = dt->next)
- gfc_copy_dt_decls_ifequal (derived, dt->derived, false);
-
+ if (gfc_derived_types) {
+ dt = gfc_derived_types;
+ for (;;)
+ {
+ gfc_copy_dt_decls_ifequal (derived, dt, false);
+ if (dt->dt_next == gfc_derived_types)
+ break;
+ dt = dt->dt_next;
+ }
+ }
Is there a particular reason why you changed the loop to "for (;;)" ?
I find the old style a bit clearer and more compact. Also I think it's
more common in gfortran.
I changed the original for loop since it wasn't possible (as far as I could
see) to have an exit condition that worked now that list is circularly
linked (i.e. "dt" never becomes NULL, and testing for dt->dt_next ==
gfc_derived_types doesn't work as it would miss the final entry in the
list). So, I used for(;;) and added the exit condition explicitly into
the loop.
- for (dt = gfc_derived_types; dt; dt = dt->next)
- gfc_copy_dt_decls_ifequal (derived, dt->derived, false);
-
+ for (dt = gfc_derived_types; dt; dt = dt->dt_next)
+ {
+ gfc_copy_dt_decls_ifequal (derived, dt, false);
+ if (dt->dt_next == gfc_derived_types)
+ break;
+ }
+
which is more compact, and has the advantage that if doesn't require an "if
(gfc_derived_types)". Do you think that's a better approach?
Yes, definitely looks better to me. Note that there is another such
case further up in gfc_get_derived_type. Actually I'd also move the
declaration of dt ("gfc_symbol *dt") into the loops, in order to make
it more local (it's not used outside).
Thanks - an updated patch is attached with those changes.

While staring at the patch again there's something about the following in
trans-types.c that seems possibly wrong:

@@ -2599,16 +2598,20 @@ gfc_get_derived_type (gfc_symbol * derived, int co
ns->translated && !got_canonical;
ns = ns->sibling)
{
- dt = ns->derived_types;
- for (; dt && !canonical; dt = dt->next)
+ if (ns->derived_types)
{
- gfc_copy_dt_decls_ifequal (dt->derived, derived, true);
- if (derived->backend_decl)
- got_canonical = true;
+ for (gfc_symbol *dt = ns->derived_types; dt; dt = dt->dt_next)
+ {
+ gfc_copy_dt_decls_ifequal (dt, derived, true);
+ if (derived->backend_decl)
+ got_canonical = true;
+ if (dt->dt_next == ns->derived_types)
+ break;
+ }
}
}
}

In the original, the loop exits if "dt" becomes NULL (i.e. end of linked list
found) or if "canonical" becomes non-null. I don't see any way for "canonical"
to change within the loop though, so I suspect that the original should have
been:

for (; dt && !got_canonical; dt = dt->next)

i.e. tested "got_canonical" instead of "canonical".

Currently my patch just removes the test of "canonical", and passes all tests
cleanly. If I'm correct about this, the original, wrong test didn't cause any
problems (since "canonical" is always NULL in this loop), but just means that
there's no early exit from the loop once the canonical type is found.

If you agree with that I can add a "!got_canonical" test back in to my patch
(I've already checked that it passes tests cleanly with that test in place).
Post by Janus Weil
Btw, another thing that you'll need is a ChangeLog entry that lists
every file and function touched by your patch and gives a short
description of the modifications. You'll find plenty of examples for
this in gcc/fortran/ChangeLog.
I've also attached a ChangeLog entry.

One other question: for copyright assignment, who do I need to talk to to get
the relevant form(s)?

Thanks,
Andrew
--
* Andrew Benson: http://users.obs.carnegiescience.edu/abenson/contact.html

* Galacticus: https://bitbucket.org/abensonca/galacticus
Janus Weil
2018-06-01 06:14:04 UTC
Permalink
Post by Andrew Benson
One other question: for copyright assignment, who do I need to talk to to get
the relevant form(s)?
I think you need to send a request to ***@gnu.org and
***@gcc.gnu.org in order to get the copyright assignment form. After
the form is mailed to you, you sign it and send it back to the FSF.

Cheers,
Janus
Steve Kargl
2018-06-11 18:17:10 UTC
Permalink
Post by Janus Weil
Post by Andrew Benson
One other question: for copyright assignment, who do I need to
talk to to get the relevant form(s)?
the form is mailed to you, you sign it and send it back to the FSF.
Andrew, I see that you've asked on ***@gcc on June 1 about
Copyright forms. Has anyone responded?

I ask because my patch for PR fortran/68544 walks the
gfc_derived_types list.

https://gcc.gnu.org/ml/fortran/2018-06/msg00054.html

and Thomas has approved the patch. In my patch, I
have

+static bool
+is_dt_name (const char *name)
+{
+ gfc_dt_list *dt_list;
+
+ for (dt_list = gfc_derived_types; dt_list; dt_list = dt_list->next)
+ if (strcmp(dt_list->derived->name, name) == 0)
+ return true;
+ return false;
+}

we'll need to update this to deal with your change for a
circular linked list.
--
Steve
Andrew Benson
2018-06-11 18:28:10 UTC
Permalink
Hi Steve,

Yes - I did get a response from ***@gcc, and then got the copyright assignment
forms sent to me. I'm now waiting on my employer signing off on the waiver so
that I can return the forms. I'll be following up with them today to hopefully
move the process along. I'll let you know as soon as I get the forms
submitted.

Thanks,
Andrew
Post by Steve Kargl
Post by Janus Weil
Post by Andrew Benson
One other question: for copyright assignment, who do I need to
talk to to get the relevant form(s)?
the form is mailed to you, you sign it and send it back to the FSF.
Copyright forms. Has anyone responded?
I ask because my patch for PR fortran/68544 walks the
gfc_derived_types list.
https://gcc.gnu.org/ml/fortran/2018-06/msg00054.html
and Thomas has approved the patch. In my patch, I
have
+static bool
+is_dt_name (const char *name)
+{
+ gfc_dt_list *dt_list;
+
+ for (dt_list = gfc_derived_types; dt_list; dt_list = dt_list->next)
+ if (strcmp(dt_list->derived->name, name) == 0)
+ return true;
+ return false;
+}
we'll need to update this to deal with your change for a
circular linked list.
--
* Andrew Benson: http://users.obs.carnegiescience.edu/abenson/contact.html

* Galacticus: https://bitbucket.org/abensonca/galacticus
Andrew Benson
2018-06-13 22:55:59 UTC
Permalink
Steve,

I have just sent my copyright assignment documents back to the FSF, so I think
it should be ok to submit my patch as soon as it has approval.

-Andrew
Post by Steve Kargl
Post by Janus Weil
Post by Andrew Benson
One other question: for copyright assignment, who do I need to
talk to to get the relevant form(s)?
the form is mailed to you, you sign it and send it back to the FSF.
Copyright forms. Has anyone responded?
I ask because my patch for PR fortran/68544 walks the
gfc_derived_types list.
https://gcc.gnu.org/ml/fortran/2018-06/msg00054.html
and Thomas has approved the patch. In my patch, I
have
+static bool
+is_dt_name (const char *name)
+{
+ gfc_dt_list *dt_list;
+
+ for (dt_list = gfc_derived_types; dt_list; dt_list = dt_list->next)
+ if (strcmp(dt_list->derived->name, name) == 0)
+ return true;
+ return false;
+}
we'll need to update this to deal with your change for a
circular linked list.
--
* Andrew Benson: http://users.obs.carnegiescience.edu/abenson/contact.html

* Galacticus: https://bitbucket.org/abensonca/galacticus
Steve Kargl
2018-06-15 00:10:24 UTC
Permalink
Ping me when you hear back from FSF. I'll apply your
current patch on the weekend to my tree and do some
testing.
--
steve
Post by Andrew Benson
Steve,
I have just sent my copyright assignment documents back to the FSF, so I think
it should be ok to submit my patch as soon as it has approval.
-Andrew
Post by Steve Kargl
Post by Janus Weil
Post by Andrew Benson
One other question: for copyright assignment, who do I need to
talk to to get the relevant form(s)?
the form is mailed to you, you sign it and send it back to the FSF.
Copyright forms. Has anyone responded?
I ask because my patch for PR fortran/68544 walks the
gfc_derived_types list.
https://gcc.gnu.org/ml/fortran/2018-06/msg00054.html
and Thomas has approved the patch. In my patch, I
have
+static bool
+is_dt_name (const char *name)
+{
+ gfc_dt_list *dt_list;
+
+ for (dt_list = gfc_derived_types; dt_list; dt_list = dt_list->next)
+ if (strcmp(dt_list->derived->name, name) == 0)
+ return true;
+ return false;
+}
we'll need to update this to deal with your change for a
circular linked list.
--
* Andrew Benson: http://users.obs.carnegiescience.edu/abenson/contact.html
* Galacticus: https://bitbucket.org/abensonca/galacticus
--
Steve
20170425

20161221

Andrew Benson
2018-06-15 00:11:20 UTC
Permalink
I'll do that.

-Andrew
Post by Steve Kargl
Ping me when you hear back from FSF. I'll apply your
current patch on the weekend to my tree and do some
testing.
Post by Andrew Benson
Steve,
I have just sent my copyright assignment documents back to the FSF, so I
think it should be ok to submit my patch as soon as it has approval.
-Andrew
Post by Steve Kargl
2018-05-31 20:04 GMT+02:00 Andrew Benson
Post by Andrew Benson
One other question: for copyright assignment, who do I need to
talk to to get the relevant form(s)?
the form is mailed to you, you sign it and send it back to the FSF.
Copyright forms. Has anyone responded?
I ask because my patch for PR fortran/68544 walks the
gfc_derived_types list.
https://gcc.gnu.org/ml/fortran/2018-06/msg00054.html
and Thomas has approved the patch. In my patch, I
have
+static bool
+is_dt_name (const char *name)
+{
+ gfc_dt_list *dt_list;
+
+ for (dt_list = gfc_derived_types; dt_list; dt_list = dt_list->next)
+ if (strcmp(dt_list->derived->name, name) == 0)
+ return true;
+ return false;
+}
we'll need to update this to deal with your change for a
circular linked list.
--
* Andrew Benson: http://users.obs.carnegiescience.edu/abenson/contact.html

* Galacticus: https://bitbucket.org/abensonca/galacticus
Andrew Benson
2018-07-11 02:32:58 UTC
Permalink
I now finally have heard back from the FSF, so my paperwork is all signed and
recorded.

I've tested my patch on the latest trunk - slightly updated version which
applies cleanly to trunk is attached (along with the ChangeLog).

-Andrew
Post by Steve Kargl
Ping me when you hear back from FSF. I'll apply your
current patch on the weekend to my tree and do some
testing.
Post by Andrew Benson
Steve,
I have just sent my copyright assignment documents back to the FSF, so I
think it should be ok to submit my patch as soon as it has approval.
-Andrew
Post by Steve Kargl
2018-05-31 20:04 GMT+02:00 Andrew Benson
Post by Andrew Benson
One other question: for copyright assignment, who do I need to
talk to to get the relevant form(s)?
the form is mailed to you, you sign it and send it back to the FSF.
Copyright forms. Has anyone responded?
I ask because my patch for PR fortran/68544 walks the
gfc_derived_types list.
https://gcc.gnu.org/ml/fortran/2018-06/msg00054.html
and Thomas has approved the patch. In my patch, I
have
+static bool
+is_dt_name (const char *name)
+{
+ gfc_dt_list *dt_list;
+
+ for (dt_list = gfc_derived_types; dt_list; dt_list = dt_list->next)
+ if (strcmp(dt_list->derived->name, name) == 0)
+ return true;
+ return false;
+}
we'll need to update this to deal with your change for a
circular linked list.
--
* Andrew Benson: http://users.obs.carnegiescience.edu/abenson/contact.html

* Galacticus: https://bitbucket.org/abensonca/galacticus
Andrew Benson
2018-07-20 16:29:19 UTC
Permalink
Pinging to see if anyone can take a look at this.

Thanks!

-Andrew
Post by Andrew Benson
I now finally have heard back from the FSF, so my paperwork is all signed
and recorded.
I've tested my patch on the latest trunk - slightly updated version which
applies cleanly to trunk is attached (along with the ChangeLog).
-Andrew
Post by Steve Kargl
Ping me when you hear back from FSF. I'll apply your
current patch on the weekend to my tree and do some
testing.
Post by Andrew Benson
Steve,
I have just sent my copyright assignment documents back to the FSF, so I
think it should be ok to submit my patch as soon as it has approval.
-Andrew
Post by Steve Kargl
2018-05-31 20:04 GMT+02:00 Andrew Benson
Post by Andrew Benson
One other question: for copyright assignment, who do I need to
talk to to get the relevant form(s)?
the form is mailed to you, you sign it and send it back to the FSF.
Copyright forms. Has anyone responded?
I ask because my patch for PR fortran/68544 walks the
gfc_derived_types list.
https://gcc.gnu.org/ml/fortran/2018-06/msg00054.html
and Thomas has approved the patch. In my patch, I
have
+static bool
+is_dt_name (const char *name)
+{
+ gfc_dt_list *dt_list;
+
+ for (dt_list = gfc_derived_types; dt_list; dt_list = dt_list->next)
+ if (strcmp(dt_list->derived->name, name) == 0)
+ return true;
+ return false;
+}
we'll need to update this to deal with your change for a
circular linked list.
--
* Andrew Benson: http://users.obs.carnegiescience.edu/abenson/contact.html

* Galacticus: https://bitbucket.org/abensonca/galacticus
Janus Weil
2018-07-20 18:59:38 UTC
Permalink
Hi Andrew,
Post by Andrew Benson
Pinging to see if anyone can take a look at this.
I think your patch has received a reasonable amount of reviewing
already. You seem to have responded to all the points that came up,
and I personally I don't see any further ones.

I had even tested your patch a few weeks ago. It did not bring much
speedup for the codes I've tried it on (probably because the number of
derived types is significantly lower than in your case), but it also
did not show any negative side effects either.

So, I'd say the patch is ok for trunk!

Do you want me to commit it for you, or do you already have a
sourceware account for svn access (see
https://gcc.gnu.org/svnwrite.html#authenticated) and prefer to do it
yourself?

Cheers,
Janus
Post by Andrew Benson
Post by Andrew Benson
I now finally have heard back from the FSF, so my paperwork is all signed
and recorded.
I've tested my patch on the latest trunk - slightly updated version which
applies cleanly to trunk is attached (along with the ChangeLog).
-Andrew
Post by Steve Kargl
Ping me when you hear back from FSF. I'll apply your
current patch on the weekend to my tree and do some
testing.
Post by Andrew Benson
Steve,
I have just sent my copyright assignment documents back to the FSF, so I
think it should be ok to submit my patch as soon as it has approval.
-Andrew
Post by Steve Kargl
2018-05-31 20:04 GMT+02:00 Andrew Benson
Post by Andrew Benson
One other question: for copyright assignment, who do I need to
talk to to get the relevant form(s)?
the form is mailed to you, you sign it and send it back to the FSF.
Copyright forms. Has anyone responded?
I ask because my patch for PR fortran/68544 walks the
gfc_derived_types list.
https://gcc.gnu.org/ml/fortran/2018-06/msg00054.html
and Thomas has approved the patch. In my patch, I
have
+static bool
+is_dt_name (const char *name)
+{
+ gfc_dt_list *dt_list;
+
+ for (dt_list = gfc_derived_types; dt_list; dt_list = dt_list->next)
+ if (strcmp(dt_list->derived->name, name) == 0)
+ return true;
+ return false;
+}
we'll need to update this to deal with your change for a
circular linked list.
--
* Andrew Benson: http://users.obs.carnegiescience.edu/abenson/contact.html
* Galacticus: https://bitbucket.org/abensonca/galacticus
Andrew Benson
2018-07-20 19:02:53 UTC
Permalink
Hi Janus,

Thanks!

I don't have a sourceware account yet - I should probably work on getting one.
But, I'm happy for you to commit the patch if you're willing to do that.

Cheers,
Andrew
Post by Janus Weil
Hi Andrew,
Post by Andrew Benson
Pinging to see if anyone can take a look at this.
I think your patch has received a reasonable amount of reviewing
already. You seem to have responded to all the points that came up,
and I personally I don't see any further ones.
I had even tested your patch a few weeks ago. It did not bring much
speedup for the codes I've tried it on (probably because the number of
derived types is significantly lower than in your case), but it also
did not show any negative side effects either.
So, I'd say the patch is ok for trunk!
Do you want me to commit it for you, or do you already have a
sourceware account for svn access (see
https://gcc.gnu.org/svnwrite.html#authenticated) and prefer to do it
yourself?
Cheers,
Janus
Post by Andrew Benson
Post by Andrew Benson
I now finally have heard back from the FSF, so my paperwork is all signed
and recorded.
I've tested my patch on the latest trunk - slightly updated version which
applies cleanly to trunk is attached (along with the ChangeLog).
-Andrew
Post by Steve Kargl
Ping me when you hear back from FSF. I'll apply your
current patch on the weekend to my tree and do some
testing.
Post by Andrew Benson
Steve,
I have just sent my copyright assignment documents back to the FSF, so I
think it should be ok to submit my patch as soon as it has approval.
-Andrew
Post by Steve Kargl
2018-05-31 20:04 GMT+02:00 Andrew Benson
Post by Andrew Benson
One other question: for copyright assignment, who do I need to
talk to to get the relevant form(s)?
the form is mailed to you, you sign it and send it back to the FSF.
Copyright forms. Has anyone responded?
I ask because my patch for PR fortran/68544 walks the
gfc_derived_types list.
https://gcc.gnu.org/ml/fortran/2018-06/msg00054.html
and Thomas has approved the patch. In my patch, I
have
+static bool
+is_dt_name (const char *name)
+{
+ gfc_dt_list *dt_list;
+
+ for (dt_list = gfc_derived_types; dt_list; dt_list = dt_list->next)
+ if (strcmp(dt_list->derived->name, name) == 0)
+ return true;
+ return false;
+}
we'll need to update this to deal with your change for a
circular linked list.
--
* Andrew Benson: http://users.obs.carnegiescience.edu/abenson/contact.html
* Galacticus: https://bitbucket.org/abensonca/galacticus
--
* Andrew Benson: http://users.obs.carnegiescience.edu/abenson/contact.html

* Galacticus: https://bitbucket.org/abensonca/galacticus
Janus Weil
2018-07-20 19:10:33 UTC
Permalink
Post by Andrew Benson
I don't have a sourceware account yet - I should probably work on getting one.
well, that's neither mandatory nor urgent, but if you plan to make
further contributions in the future, it definitely makes sense to get
one.
Post by Andrew Benson
But, I'm happy for you to commit the patch if you're willing to do that.
Sure, no problem. Should be able to get that done within the next hour ...

Cheers,
Janus
Post by Andrew Benson
Post by Janus Weil
Hi Andrew,
Post by Andrew Benson
Pinging to see if anyone can take a look at this.
I think your patch has received a reasonable amount of reviewing
already. You seem to have responded to all the points that came up,
and I personally I don't see any further ones.
I had even tested your patch a few weeks ago. It did not bring much
speedup for the codes I've tried it on (probably because the number of
derived types is significantly lower than in your case), but it also
did not show any negative side effects either.
So, I'd say the patch is ok for trunk!
Do you want me to commit it for you, or do you already have a
sourceware account for svn access (see
https://gcc.gnu.org/svnwrite.html#authenticated) and prefer to do it
yourself?
Cheers,
Janus
Post by Andrew Benson
Post by Andrew Benson
I now finally have heard back from the FSF, so my paperwork is all signed
and recorded.
I've tested my patch on the latest trunk - slightly updated version which
applies cleanly to trunk is attached (along with the ChangeLog).
-Andrew
Post by Steve Kargl
Ping me when you hear back from FSF. I'll apply your
current patch on the weekend to my tree and do some
testing.
Post by Andrew Benson
Steve,
I have just sent my copyright assignment documents back to the FSF, so I
think it should be ok to submit my patch as soon as it has approval.
-Andrew
Post by Steve Kargl
2018-05-31 20:04 GMT+02:00 Andrew Benson
Post by Andrew Benson
One other question: for copyright assignment, who do I need to
talk to to get the relevant form(s)?
After
the form is mailed to you, you sign it and send it back to the FSF.
Copyright forms. Has anyone responded?
I ask because my patch for PR fortran/68544 walks the
gfc_derived_types list.
https://gcc.gnu.org/ml/fortran/2018-06/msg00054.html
and Thomas has approved the patch. In my patch, I
have
+static bool
+is_dt_name (const char *name)
+{
+ gfc_dt_list *dt_list;
+
+ for (dt_list = gfc_derived_types; dt_list; dt_list = dt_list->next)
+ if (strcmp(dt_list->derived->name, name) == 0)
+ return true;
+ return false;
+}
we'll need to update this to deal with your change for a
circular linked list.
--
* Andrew Benson: http://users.obs.carnegiescience.edu/abenson/contact.html
* Galacticus: https://bitbucket.org/abensonca/galacticus
--
* Andrew Benson: http://users.obs.carnegiescience.edu/abenson/contact.html
* Galacticus: https://bitbucket.org/abensonca/galacticus
Janus Weil
2018-07-20 20:04:47 UTC
Permalink
Post by Janus Weil
Post by Andrew Benson
But, I'm happy for you to commit the patch if you're willing to do that.
Sure, no problem. Should be able to get that done within the next hour ...
Ok, I have made some small last-minute adjustments (fixed an overlong
line and some whitespace issues and brushed up the ChangeLog a bit)
and committed the patch as r262909:

https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=262909

Not much left to say, except: Congratulations to your first GCC
contribution, welcome to the gfortran team and thanks for the nice
work! We hope there is more of that to come :)

Cheers,
Janus
Post by Janus Weil
Post by Andrew Benson
Post by Janus Weil
Hi Andrew,
Post by Andrew Benson
Pinging to see if anyone can take a look at this.
I think your patch has received a reasonable amount of reviewing
already. You seem to have responded to all the points that came up,
and I personally I don't see any further ones.
I had even tested your patch a few weeks ago. It did not bring much
speedup for the codes I've tried it on (probably because the number of
derived types is significantly lower than in your case), but it also
did not show any negative side effects either.
So, I'd say the patch is ok for trunk!
Do you want me to commit it for you, or do you already have a
sourceware account for svn access (see
https://gcc.gnu.org/svnwrite.html#authenticated) and prefer to do it
yourself?
Cheers,
Janus
Post by Andrew Benson
Post by Andrew Benson
I now finally have heard back from the FSF, so my paperwork is all signed
and recorded.
I've tested my patch on the latest trunk - slightly updated version which
applies cleanly to trunk is attached (along with the ChangeLog).
-Andrew
Post by Steve Kargl
Ping me when you hear back from FSF. I'll apply your
current patch on the weekend to my tree and do some
testing.
Post by Andrew Benson
Steve,
I have just sent my copyright assignment documents back to the FSF, so I
think it should be ok to submit my patch as soon as it has approval.
-Andrew
Post by Steve Kargl
2018-05-31 20:04 GMT+02:00 Andrew Benson
Post by Andrew Benson
One other question: for copyright assignment, who do I need to
talk to to get the relevant form(s)?
After
the form is mailed to you, you sign it and send it back to the
FSF.
Copyright forms. Has anyone responded?
I ask because my patch for PR fortran/68544 walks the
gfc_derived_types list.
https://gcc.gnu.org/ml/fortran/2018-06/msg00054.html
and Thomas has approved the patch. In my patch, I
have
+static bool
+is_dt_name (const char *name)
+{
+ gfc_dt_list *dt_list;
+
+ for (dt_list = gfc_derived_types; dt_list; dt_list =
dt_list->next)
+ if (strcmp(dt_list->derived->name, name) == 0)
+ return true;
+ return false;
+}
we'll need to update this to deal with your change for a
circular linked list.
--
* Andrew Benson: http://users.obs.carnegiescience.edu/abenson/contact.html
* Galacticus: https://bitbucket.org/abensonca/galacticus
--
* Andrew Benson: http://users.obs.carnegiescience.edu/abenson/contact.html
* Galacticus: https://bitbucket.org/abensonca/galacticus
Andrew Benson
2018-07-20 20:21:57 UTC
Permalink
Thanks! I have a few other areas I've identified that look promising for
similar optimizations, so I'm hoping to look into those soon.

Cheers,
Andrew
Post by Janus Weil
Post by Janus Weil
Post by Andrew Benson
But, I'm happy for you to commit the patch if you're willing to do that.
Sure, no problem. Should be able to get that done within the next hour ...
Ok, I have made some small last-minute adjustments (fixed an overlong
line and some whitespace issues and brushed up the ChangeLog a bit)
https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=262909
Not much left to say, except: Congratulations to your first GCC
contribution, welcome to the gfortran team and thanks for the nice
work! We hope there is more of that to come :)
Cheers,
Janus
Post by Janus Weil
Post by Andrew Benson
Post by Janus Weil
Hi Andrew,
Post by Andrew Benson
Pinging to see if anyone can take a look at this.
I think your patch has received a reasonable amount of reviewing
already. You seem to have responded to all the points that came up,
and I personally I don't see any further ones.
I had even tested your patch a few weeks ago. It did not bring much
speedup for the codes I've tried it on (probably because the number of
derived types is significantly lower than in your case), but it also
did not show any negative side effects either.
So, I'd say the patch is ok for trunk!
Do you want me to commit it for you, or do you already have a
sourceware account for svn access (see
https://gcc.gnu.org/svnwrite.html#authenticated) and prefer to do it
yourself?
Cheers,
Janus
Post by Andrew Benson
Post by Andrew Benson
I now finally have heard back from the FSF, so my paperwork is all signed
and recorded.
I've tested my patch on the latest trunk - slightly updated version which
applies cleanly to trunk is attached (along with the ChangeLog).
-Andrew
Post by Steve Kargl
Ping me when you hear back from FSF. I'll apply your
current patch on the weekend to my tree and do some
testing.
Post by Andrew Benson
Steve,
I have just sent my copyright assignment documents back to the
FSF,
so I
think it should be ok to submit my patch as soon as it has approval.
-Andrew
Post by Steve Kargl
2018-05-31 20:04 GMT+02:00 Andrew Benson
Post by Andrew Benson
One other question: for copyright assignment, who do I need
to
talk to to get the relevant form(s)?
form.
After
the form is mailed to you, you sign it and send it back to
the
FSF.
Copyright forms. Has anyone responded?
I ask because my patch for PR fortran/68544 walks the
gfc_derived_types list.
https://gcc.gnu.org/ml/fortran/2018-06/msg00054.html
and Thomas has approved the patch. In my patch, I
have
+static bool
+is_dt_name (const char *name)
+{
+ gfc_dt_list *dt_list;
+
+ for (dt_list = gfc_derived_types; dt_list; dt_list =
dt_list->next)
+ if (strcmp(dt_list->derived->name, name) == 0)
+ return true;
+ return false;
+}
we'll need to update this to deal with your change for a
circular linked list.
--
http://users.obs.carnegiescience.edu/abenson/contact.html
* Galacticus: https://bitbucket.org/abensonca/galacticus
--
http://users.obs.carnegiescience.edu/abenson/contact.html
* Galacticus: https://bitbucket.org/abensonca/galacticus
--
* Andrew Benson: http://users.obs.carnegiescience.edu/abenson/contact.html

* Galacticus: https://bitbucket.org/abensonca/galacticus
Loading...