Re: [Patch, fortran] PR46897 - [OOP] type-bound defined ASSIGNMENT(=) not used for derived type component in intrinsic assign

[Paul Richard Thomas <paul dot richard dot thomas at gmail dot com>]

Dear Mikael,

Thank you for the last review of my patch for this PR.  Since then, I
have had difficulty to find time for gfortran for both personal and
professional reasons.  Anyway, the attached is my attempt to remedy
the problems that you identified.

In a moment of madness, I clicked on "remove trailing white space"...
well, you can see the result!  Fortunately, all the meat of the patch
in a contiguous chunk so you just have to search for "add_comp_ref"
and you are there.

I THINK that this is pretty complete but you all proved me wrong last
time, so I am just awaiting the contradictions to that statement :-(

Array sections on the lhs are not handled as well as might be hoped,
when an lhs temporary is needed; the temporary comprises the full
array and the section is explicitly referenced.  Fortunately, it is
rare that memory is a problem these days so I have let it be.  Should
need arise, the temporary could be made allocatable and ALLOCATE
called explicitly (my attempts to use the reallocate on assign
mechanism failed for reasons that I could not see.  I'll try again
some other time.)

Note that I have had to punt on defined assignments when there is more
than one part reference involved.  The warning message suggests making
the loops explicit to make the defined assignment work.

I have made responses to your review below.  I also fixed the testcase
that Tobias posted in the subsequent correspondence.

Bootstrapped and regtested on x86_64/FC17 - OK for trunk?

Cheers

Paul

2012-11-18   Alessandro Fanfarillo <alessandro.fanfarillo@gmail.com>
             Paul Thomas  <pault@gcc.gnu.org>

	PR fortran/46897
	* gfortran.h : Add bit field 'defined_assign_comp' to
	symbol_attribute structure.
	Add primitive for gfc_add_full_array_ref.
	* expr.c (gfc_add_full_array_ref): New function.
	(gfc_lval_expr_from_sym): Call new function.
	* resolve.c (add_comp_ref): New function.
	(build_assignment): New function.
	(get_temp_from_expr): New function
	(add_code_to_chain): New function
	(generate_component_assignments): New function that calls all
	the above new functions.
	(resolve_code): Call generate_component_assignments.
	(check_defined_assignments): New function.
	(resolve_fl_derived0): Call check_defined_assignments.
	(gfc_resolve): Reset component_assignment_level in case it is
	left in a bad state by errors.


	* resolve.c (is_sym_host_assoc, resolve_procedure_interface,
	resolve_contained_fntype, resolve_procedure_expression,
	resolve_elemental_actual, resolve_global_procedure,
	is_scalar_expr_ptr, gfc_iso_c_func_interface, resolve_function,
	set_name_and_label, gfc_iso_c_sub_interface,
	resolve_specific_s0, resolve_operator, compare_bound_mpz_t,
	gfc_resolve_character_operator, resolve_typebound_function,
	gfc_resolve_expr, forall_index, remove_last_array_ref,
	conformable_arrays, resolve_allocate_expr,
	resolve_allocate_deallocate, resolve_select_type,
	resolve_transfer, resolve_where,
	gfc_resolve_where_code_in_forall, gfc_resolve_forall_body,
	gfc_count_forall_iterators, resolve_values,
	resolve_bind_c_comms, resolve_bind_c_derived_types,
	gfc_verify_binding_labels, apply_default_init,
	build_default_init_expr, apply_default_init_local,
	resolve_fl_var_and_proc, resolve_fl_procedure,
	gfc_resolve_finalizers, check_generic_tbp_ambiguity,
	resolve_typebound_intrinsic_op, resolve_typebound_procedure,
	resolve_typebound_procedures, ensure_not_abstract,
	resolve_fl_derived0, resolve_fl_parameter, resolve_symbol,
	resolve_equivalence_derived): Remove trailing white space.
	* gfortran.h : Remove trailing white space.

2012-01-18   Alessandro Fanfarillo <alessandro.fanfarillo@gmail.com>
             Paul Thomas  <pault@gcc.gnu.org>

	PR fortran/46897
	* gfortran.dg/defined_assignment_1.f90: New test.
	* gfortran.dg/defined_assignment_2.f90: New test.
	* gfortran.dg/defined_assignment_3.f90: New test.
	* gfortran.dg/defined_assignment_4.f90: New test.
	* gfortran.dg/defined_assignment_5.f90: New test.


On 17 September 2012 20:45, Mikael Morin <mikael.morin@sfr.fr> wrote:
> Hello,
....
> And here comes the next round of comments.
>
....
>> +   /* Add a full array ref, as necessary.  */
>> +   e->rank = c && c->as ? c->as->rank : 0;
> This is bogus if  e->rank was != 0 previously (think of the case
> array(:)%scalar_comp).
> The c == NULL case should be handled at the beginning (if at all).
>
>> +   if (e->rank)
> the condition should be on c->as (for the case array(:)%scalar_comp again).

eliminated
....
> all calls are with op == EXEC_ASSIGN, you may as well hardcode it.

see comment - I was contemplating more general use of this code.
....
>> +   gcc_assert (e->expr_type == EXPR_VARIABLE
>> +           || e->expr_type == EXPR_FUNCTION);
> As far as I know anything can be used, not only variables and functions.
> The derived type cases are a bit specific but at least array/structure
> constructors are missing.  There could be also typebound function calls
> (I never know whether they are EXPR_FUNCTION or something else).

eliminated
....

>> +    ! Now do the defined assignments
>> +       do over components with typebound defined assignment [%cmp]
>> +     expr2%cmp {defined=} t1%cmp
> I guess it should be `t1%cmp {defined=} expr2%cmp'?
>
>> +     expr1%cmp = t1%cmp           ! Store in the result
>> +         t1%cmp = t2%cmp              ! Restore the original value
> It seems to me that the last assignment isn't useful: once one component
> has been taken care of, we proceed with the next one, so `t1' can have
> garbage in the previous one without any impact.
> Then if it can be removed, `t2' is useless and then `temp_x' as well, so
> we could do with the simpler [still most of `t1' is useless]:
>    t1 = expr1
>    expr1 = expr2
>    t1%cmp {defined=} expr1%cmp
>    expr1%cmp = t1%cmp
>
> It would be nice too if the temporaries were avoided in the case there
> is no defined assignment with intent(inout) lhs, but I leave the
> decision to do it to you.
>

This has all been corrected.
....
>> +       || comp1->attr.pointer
>> +       || comp1->attr.allocatable
>> +       || comp1->attr.proc_pointer_comp
> That one doesn't look right.

It did not look right to me either - eliminating it causes a
regression - I forget which test now.
....

>> +           gfc_free_statements (this_code);
> `this_code' should be cleared, otherwise it is used in the next iteration.

It's been nulled on each occasion.

>
> free (head); ?

Done

>> +     sym->attr.defined_assign_comp
>> +                     = super_type->attr.defined_assign_comp;
> I guess Tobias' reported bug is here.  The flag shouldn't be cleared
> here if it was set just before.

This was all put right.

>> +
>>     /* If this is a non-ABSTRACT type extending an ABSTRACT one, ensure that
>>        all DEFERRED bindings are overridden.  */
>>     if (super_type && super_type->attr.abstract && !sym->attr.abstract
>
>
> To finish, I would like to draw your attention on the scalarizer not
> supporting multiple arrays in the reference chain.  The initial
> expressions are guaranteed to have at most one array in the chain, but
> as we add subfield references, that condition can not remain true.  We
> could try adding multiple references support in the scalarizer, but I
> don't know how difficult it would be.  Or maybe better fix it at the
> front-end AST level by using elemental functions to split the
> scalarization work.  Or something else.  What do you think?

See the gfc_warning; If multiple array references are encountered, no
attempt is made to use the defined assignments and the user is advised
to make the (outer) loop explicit.

With best regards

Paul

Attachment: resubmit.diff
Description: Binary data