Optimization of add_dt_to_dt_list() in resolve.c

[Andrew Benson]

On Thursday, May 31, 2018 10:56:47 AM PDT Janus Weil wrote:
> 2018-05-31 0:22 GMT+02:00 Andrew Benson <abenson@carnegiescience.edu>:
> >> -  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?
> 
> 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).

> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch.diff
Type: text/x-patch
Size: 8567 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/fortran/attachments/20180531/9d19133a/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ChangeLog
Type: text/x-changelog
Size: 808 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/fortran/attachments/20180531/9d19133a/attachment-0001.bin>