Re: [Patch, fortran] PR69834 - Collision in derived type hashes

[Andre Vehreschild <vehre@gmx.de>]

Hi Paul,

here are my comments to your patch:

> Index: gcc/fortran/class.c
> ===================================================================
> *** gcc/fortran/class.c (revision 241439)
> --- gcc/fortran/class.c (working copy)
> *************** add_procs_to_declared_vtab (gfc_symbol *
> --- 2187,2219 ----
>   gfc_symbol *
>   gfc_find_derived_vtab (gfc_symbol *derived)
>   {
> !   gfc_namespace *ns = NULL;

Setting this to NULL for consistency?

> Index: gcc/fortran/dump-parse-tree.c
> ===================================================================
> *** gcc/fortran/dump-parse-tree.c       (revision 241439)
> --- gcc/fortran/dump-parse-tree.c       (working copy)
> *************** show_code_node (int level, gfc_code *c)
> *** 1843,1848 ****
> --- 1843,1877 ----

Well, the code in this chunk is identical to the one of EXEC_SELECT, besides
two lines where the statement's name is printed. I propose to do something like:

case EXEC_SELECT:
case EXEC_SELECT_TYPE:
  d= ..
  fputs ("SELECT", dumpfile);
  if (c->op == EXEC_SELECT_TYPE)
    fputs (" TYPE", dumpfile);
 ...
  // and the same for "END SELECT..."

This would reduce the amount of copied code. An improvement in one
EXEC_SELECT-dump-handler would then automagically available in the other, too.

> Index: gcc/fortran/resolve.c
> ===================================================================
> *** gcc/fortran/resolve.c       (revision 241439)
> --- gcc/fortran/resolve.c       (working copy)
<snipp>
> *************** resolve_select_type (gfc_code *code, gfc
<snipp>
> --- 8595,8641 ----
>     else
>       ns->code->next = new_st;
>     code = new_st;
> !   code->op = EXEC_SELECT_TYPE;
> 
> +   /* Use the intrinsic LOC function to generate the an integer expression
> +      for the vtable of the selector.  Note that the rank of the selector
> +      expression has to be set to zero.  */

double article:                                    _the an_  !!!

> Index: gcc/fortran/trans-stmt.c
> ===================================================================
> *** gcc/fortran/trans-stmt.c    (revision 241439)
> --- gcc/fortran/trans-stmt.c    (working copy)
> *************** gfc_trans_do_while (gfc_code * code)
> *** 2331,2336 ****
> --- 2331,2455 ----
<snipp>
> +
> +   /* Translate an assignment to a CLASS object
> +      (pointer or ordinary assignment).  */
> +
> +

Here is no routine the above comment could document. Left over from prior
version?

>   /* End of prototype trans-class.c  */


> Index: gcc/fortran/trans-stmt.c
> ===================================================================
> *** gcc/fortran/trans-stmt.c    (revision 241439)
> --- gcc/fortran/trans-stmt.c    (working copy)
> *************** gfc_trans_do_while (gfc_code * code)
> *** 2331,2336 ****
> --- 2331,2455 ----

Can one optimize this by using the "old style" for intrinsic types, i.e. a
computed goto (switch-case) for them? And in the default case the if-chain on
the derived types/classes? Would we gain any speed by this? What is your
opinion on this?

> Please find attached a revised version of the patch that corrects one
> or two tiny wrinkles. I have removed the tidy up of vtable retrieval

I haven't understood yet, what you need to do for this. Looking forward to that
patch.

With the above small changes the patch is ok for trunk given that Dominique
doesn't find any issues.

Beware, that my big patch on polymorphic assignment will *not* be backported
to gcc-6. I.e., this version of your patch will most probably not be applyable.
You rather will need to apply the old version.

Thanks for the work.

Regards,
	Andre

> Functionally, the patch is as described in the original submission.
> 
> As attached, it bootstraps and regtests on FC21/x86_64.  OK for trunk
> and, after a decent interval for 6-branch?
> 
> Cheers
> 
> Paul
> 
> 2016-10-22  Paul Thomas  <pault@gcc.gnu.org>
> 
>     PR fortran/69834
>     * class.c (gfc_find_derived_vtab): Obtain the gsymbol for the
>     derived type's module. If the gsymbol is present and the top
>     level namespace corresponds to a module, use the gsymbol name
>     space. In the search to see if the vtable exists, try the gsym
>     namespace first.
>     * dump-parse-tree (show_code_node): Add explicit dump for the
>     select type construct.
>     * resolve.c (build_loc_call): New function.
>     (resolve_select_type): Add check for repeated type is cases.
>     Retain selector expression and use it later instead of expr1.
>     Exclude deferred length TYPE IS cases and emit error message.
>     Store the address for the vtable in the 'low' expression and
>     the hash value in the 'high' expression, for each case. Do not
>     call resolve_select.
>     * trans.c(trans_code) : Call gfc_trans_select_type.
>     * trans-stmt.c (gfc_trans_select_type_cases): New function.
>     (gfc_trans_select_type): New function.
>     * trans-stmt.h : Add prototype for gfc_trans_select_type.
> 
> 2016-10-22  Paul Thomas  <pault@gcc.gnu.org>
> 
>     PR fortran/69834
>     * gfortran.dg/select_type_1.f03: Change error for overlapping
>     TYPE IS cases.
>     * gfortran.dg/select_type_36.f03: New test.


-- 
Andre Vehreschild * Email: vehre ad gmx dot de