[m-rev.] for post-commit review: Match output arguments correctly for mutually recursive code

[Zoltan Somogyi]

On Tue, 11 Apr 2017 15:21:49 +1000, Paul Bone <paul at bone.id.au> wrote:
> +is_output_arg_rename(ToVar, FromVar, [Var0 | Vars0], [Var | Vars]) :-
> +    % The assignment assings _to_ ToVar, so we went to map _from_ ToVar back
> +    % to FromVar.
> +    ( if ToVar = Var0 then
> +        Var = FromVar,
> +        Vars = Vars0
> +    else
> +        % The original code didn't keep searching if the first output
> +        % variable wasn't the one being renamed.  That seems incorrect.
> +        %  -pbone.
> +        Var = Var0,
> +        is_output_arg_rename(ToVar, FromVar, Vars0, Vars)
> +    ).

Thanks for catching that.
 
> +match_output_args_2(_, OutputVars, [], [], [], Match) :-
> +    require_det
>      (
> -        MaybeOutputVar = no
> +        OutputVars = [],
> +        Match = outputs_match
>      ;
> -        MaybeOutputVar = yes(ArgVar)
> -    ),
> -    match_output_args(MaybeOutputVars, ArgVars).
> +        OutputVars = [_ | _],
> +        Match = outputs_dont_match
> +    ).
> +match_output_args_2(Info, OutputVars0, [ArgVar | ArgVars],
> +        [ArgType | ArgTypes], [ArgMode | ArgModes], Match) :-
> +    (
> +        OutputVars0 = [],
> +        % Any remaning arguments (of the call site) don't matter, if they're
> +        % outputs those outputs are simply ignored.
> +        Match = outputs_match
> +    ;
> +        OutputVars0 = [OutputVar | OutputVars],
> +        ( if is_output(Info ^ mtc_module, ArgMode, ArgType) then
> +            ( if OutputVar = ArgVar then
> +                match_output_args_2(Info, OutputVars, ArgVars, ArgTypes,
> +                    ArgModes, Match)
> +            else
> +                Match = outputs_dont_match
> +            )
> +        else
> +            % Don't consume the current output if the current ArgVar is not
> +            % an output.
> +            match_output_args_2(Info, OutputVars0, ArgVars, ArgTypes,
> +                ArgModes, Match)
> +        )
> +    ).

I don't think ignoring the remaining arguments in the OutputVars0 = [] case
is right. If the caller's output arguments are X, Y and Z, and the callee's are
X, Y, Z and W, then I don't think we want to consider the call to be a tail call.
The code we would generate for a tail call for e.g. the LLDS backend may do
the right thing, but it would do so only by accident, and I don't think we
want to rely on accidents.

I think requiring the caller and callee to have the *same* sequence of
output arguments is safer, and it can be done with simpler code.
That is why I am committing the attached diff.

Zoltan.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: DIFF.apr12
Type: application/octet-stream
Size: 8876 bytes
Desc: not available
URL: <http://lists.mercurylang.org/archives/reviews/attachments/20170412/4be5e49c/attachment.obj>