[Patch, fortran] PR64952 - Missing temporary in assignment from elemental function

[Paul Richard Thomas]

Dear Mikael,

Thank you very much for the review. You raise some points that I had
thought about and others that I hadn't. I also realised that such
things as blocks, within the elemental function would through the fix
as well. I'll defer doing anything with it until tomorrow night.

I reason that there is always going to be an 'ss', although I should
check that it is not gfc_ss_terminator, and that it does not matter
which one is flagged. I should add a comment to that effect; it's not
quite as hackish as it looks, methinks.

I will be back!

Paul

On 8 February 2015 at 18:27, Mikael Morin <mikael.morin@sfr.fr> wrote:
> Hello Paul,
>
> comments below
>
> Le 08/02/2015 16:24, Paul Richard Thomas a écrit :
>>
>> Index: gcc/fortran/gfortran.h
>> ===================================================================
>> *** gcc/fortran/gfortran.h    (revision 220482)
>> --- gcc/fortran/gfortran.h    (working copy)
>> *************** typedef struct
>> *** 789,794 ****
>> --- 789,798 ----
>>        cannot alias.  Note that this is zero for PURE procedures.  */
>>     unsigned implicit_pure:1;
>>
>> +   /* This set for an elemental function that contains expressions for
>> +      arrays coming from outside its namespace.  */
>> +   unsigned potentially_aliased:1;
>> +
> aliased is more something about pointers, so how about naming it
> something like array_outer_dependency?  Anyway, that's minor.
>
> I wonder whether we should negate the meaning, that is set the flag if
> there is no external dependency.
> If we can get the conditions to set it exhaustively right, both are
> equivalent.  Otherwise... maybe not.
>
>>     /* This is set if the subroutine doesn't return.  Currently, this
>>        is only possible for intrinsic subroutines.  */
>>     unsigned noreturn:1;
>> Index: gcc/fortran/trans.h
>> ===================================================================
>> *** gcc/fortran/trans.h       (revision 220481)
>> --- gcc/fortran/trans.h       (working copy)
>> *************** typedef struct gfc_ss_info
>> *** 226,231 ****
>> --- 226,235 ----
>>     /* Suppresses precalculation of scalars in WHERE assignments.  */
>>     unsigned where:1;
>>
>> +   /* Signals that an array argument of an elemental function might be aliased,
>> +      thereby generating a temporary in assignments.  */
>> +   unsigned potentially_aliased:1;
>> +
>>     /* Tells whether the SS is for an actual argument which can be a NULL
>>        reference.  In other words, the associated dummy argument is OPTIONAL.
>>        Used to handle elemental procedures.  */
>> Index: gcc/fortran/resolve.c
>> ===================================================================
>> *** gcc/fortran/resolve.c     (revision 220481)
>> --- gcc/fortran/resolve.c     (working copy)
>> *************** resolve_variable (gfc_expr *e)
>> *** 5054,5059 ****
>> --- 5054,5067 ----
>>                   && gfc_current_ns->parent->parent == sym->ns)))
>>       sym->attr.host_assoc = 1;
>>
>> +   if (sym->attr.dimension
>> +       && (sym->ns != gfc_current_ns
>> +       || sym->attr.use_assoc
>> +       || sym->attr.in_common)
>> +       && gfc_elemental (NULL)
>> +       && gfc_current_ns->proc_name->attr.function)
>> +     gfc_current_ns->proc_name->attr.potentially_aliased = 1;
> I would expect the flag to also be copied between procedures in some
> cases; namely if A calls B, and B has the flag, then A has the flag.
> There is also the case of external procedures (for which the flag is not
> known -> assume the worst)
>
>> +
>>   resolve_procedure:
>>     if (t && !resolve_procedure_expression (e))
>>       t = false;
>> Index: gcc/fortran/trans-array.c
>> ===================================================================
>> *** gcc/fortran/trans-array.c (revision 220482)
>> --- gcc/fortran/trans-array.c (working copy)
>> *************** gfc_conv_resolve_dependencies (gfc_loopi
>> *** 4391,4396 ****
>> --- 4391,4402 ----
>>       {
>>         ss_expr = ss->info->expr;
>>
>> +       if (ss->info->potentially_aliased)
>> +     {
>> +       nDepend = 1;
>> +       break;
>> +     }
>> +
>>         if (ss->info->type != GFC_SS_SECTION)
>>       {
>>         if (flag_realloc_lhs
>> *************** gfc_walk_function_expr (gfc_ss * ss, gfc
>> *** 9096,9104 ****
>>     /* Walk the parameters of an elemental function.  For now we always pass
>>        by reference.  */
>>     if (sym->attr.elemental || (comp && comp->attr.elemental))
>> !     return gfc_walk_elemental_function_args (ss, expr->value.function.actual,
>>                                            gfc_get_proc_ifc_for_expr (expr),
>>                                            GFC_SS_REFERENCE);
>>
>>     /* Scalar functions are OK as these are evaluated outside the scalarization
>>        loop.  Pass back and let the caller deal with it.  */
>> --- 9102,9114 ----
>>     /* Walk the parameters of an elemental function.  For now we always pass
>>        by reference.  */
>>     if (sym->attr.elemental || (comp && comp->attr.elemental))
>> !     {
>> !       ss = gfc_walk_elemental_function_args (ss, expr->value.function.actual,
>>                                            gfc_get_proc_ifc_for_expr (expr),
>>                                            GFC_SS_REFERENCE);
>> +       if (sym->attr.potentially_aliased)
>> +     ss->info->potentially_aliased = 1;
>> +     }
>
> This is somewhat hackish, potentially_aliased is a global thing, not
> specific to SS, and this may end up marking gfc_ss_terminator as
> potentiallly_aliased for example, but I don't see any other obvious way
> to do it, so it's OK I guess.
>
> Anyway, the comp && comp->attr.elemental part of the if should be
> handled too (always set the flag in that case?).  I actually wonder why
> it works without.
>
> I attach a few variants of the testcase, which don't work yet.
>
> Mikael
>



-- 
Outside of a dog, a book is a man's best friend. Inside of a dog it's
too dark to read.

Groucho Marx