https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92887
--- Comment #6 from anlauf at gcc dot gnu.org ---
(In reply to Mikael Morin from comment #5)
> (In reply to anlauf from comment #4)
> >
> > I'll need broader feedback, so unless someone adds to this pr, I'll submit
> > the present patch - with testcases - to get attention.
> >
> Here you go:
>
> > diff --git a/gcc/fortran/trans-expr.cc b/gcc/fortran/trans-expr.cc
> > index 45a984b6bdb..d9dcc11e5bd 100644
> > --- a/gcc/fortran/trans-expr.cc
> > +++ b/gcc/fortran/trans-expr.cc
>
> > @@ -6396,7 +6399,28 @@ gfc_conv_procedure_call (gfc_se * se, gfc_symbol *
> > sym,
> > && fsym->ts.type != BT_CLASS
> > && fsym->ts.type != BT_DERIVED)
> > {
> > - if (e->expr_type != EXPR_VARIABLE
> > + /* F2018:15.5.2.12 Argument presence and
> > + restrictions on arguments not present. */
> > + if (e->expr_type == EXPR_VARIABLE
> > + && (e->symtree->n.sym->attr.allocatable
> > + || e->symtree->n.sym->attr.pointer))
>
> Beware of expressions like derived%alloc_comp or derived%pointer_comp which
> don't match the above.
Right. This is fixable by using
&& (gfc_expr_attr (e).allocatable
|| gfc_expr_attr (e).pointer))
instead.
> > @@ -7072,6 +7096,42 @@ gfc_conv_procedure_call (gfc_se * se, gfc_symbol *
> > sym,
> > }
> > }
> >
> > + /* F2023:15.5.3, 15.5.4: Actual argument expressions are evaluated
> > + before they are associated and a procedure is executed. */
> > + if (e && e->expr_type != EXPR_VARIABLE && !gfc_is_constant_expr (e))
> > + {
> > + /* Create temporary except for functions returning pointers that
> > + can appear in a variable definition context. */
>
> Maybe explain *why* we have to create a temporary, that is some data
> references may become undefined by the procedure call (intent(out) dummies)
> so we have to evaluate values depending on them beforehand (PR 92178).
That is one reason. Another one, also pointed out in PR92178 by Tobias' review
of Steve's draft, is the first testcase at
https://gcc.gnu.org/legacy-ml/gcc-patches/2019-10/msg01970.html
This is reminiscent to an issue reported for the MERGE intrinsic (pr107874,
fixed so far, but there is a remaining issue in pr105371).
> > + if (e->expr_type != EXPR_FUNCTION
> > + || !(gfc_expr_attr (e).pointer || gfc_expr_attr (e).proc_pointer))
>
> Merge with the outer condition?
Yes. The above form was intended more for proof-of-concept and readability
than for coding standards.
> > + need_temp = true;
> > + }
> > +
> > + if (need_temp)
> > + {
> > + if (cond_temp == NULL_TREE)
> > + parmse.expr = gfc_evaluate_now (parmse.expr, &parmse.pre);
>
> I'm not sure about this. The condition to set need_temp looks quite general
> (especially it matches non-scalar cases, doesn't it?), but
> gfc_conv_expr_reference should already take care of creating a variable, so
> that a temporary is missing only for value dummies, I think. I would rather
> move this to the place specific to value dummies.
I agree in principle. The indentation level is already awful in the specific
place, which calls for thoughts about refactoring that mega-loop over the
arguments than currently spans far more than 1000 source code lines.
> I think this PR is only about scalars with basic types, is there the same
> problem with derived types? with classes?
> I guess arrays are different as they are always by reference?
For the current documentation of the argument passing convention see:
https://gcc.gnu.org/onlinedocs/gfortran/Argument-passing-conventions.html
"For OPTIONAL dummy arguments, an absent argument is denoted by a NULL pointer,
except for scalar dummy arguments of intrinsic type which have the VALUE
attribute. For those, a hidden Boolean argument (logical(kind=C_bool),value) is
used to indicate whether the argument is present."
My understanding is that for these scalar arguments we do need something that
can be passed by value.
We currently do not support VALUE with array arguments (F2008+), character
of length > 1, and character actual arguments are broken unless they are
constants. There are several open PRs.
> > + else
>
> I would rather move the else part to the place above where cond_temp is set,
> so that the code is easier to follow.
>
> > + {
> > + /* "Conditional temporary" to handle variables that possibly
> > + cannot be dereferenced. Use null value as fallback. */
> > + tree dflt_temp;
> > + gcc_assert (e->ts.type != BT_DERIVED && e->ts.type != BT_CLASS);
> > + gcc_assert (e->rank == 0);
> > + dflt_temp = gfc_create_var (TREE_TYPE (parmse.expr), "temp");
> > + TREE_STATIC (dflt_temp) = 1;
> > + TREE_CONSTANT (dflt_temp) = 1;
> > + TREE_READONLY (dflt_temp) = 1;
> > + DECL_INITIAL (dflt_temp) = build_zero_cst (TREE_TYPE (dflt_temp));
> > + parmse.expr = fold_build3_loc (input_location, COND_EXPR,
> > + TREE_TYPE (parmse.expr),
> > + cond_temp,
> > + parmse.expr, dflt_temp);
> > + parmse.expr = gfc_evaluate_now (parmse.expr, &parmse.pre);
> > + }
> > + }
> > +
> > +
> > if (fsym && need_interface_mapping && e)
> > gfc_add_interface_mapping (&mapping, fsym, &parmse, e);
> >
I agree that it was a bad idea to merge the patches for these two PRs just
because temporaries are needed.
