[m-rev.] for review: Fix codegen for subtype field names in high-level data grades.

[Zoltan Somogyi]

On 2024-04-30 17:08 +10:00 AEST, "Peter Wang" <novalazy at gmail.com> wrote:
> diff --git a/compiler/du_type_layout.m b/compiler/du_type_layout.m
> index 64d7adbc7..f6fb17d69 100644
> --- a/compiler/du_type_layout.m
> +++ b/compiler/du_type_layout.m
> @@ -2,7 +2,7 @@
>  % vim: ft=mercury ts=4 sw=4 et
>  %---------------------------------------------------------------------------%
>  % Copyright (C) 1993-2012 The University of Melbourne.
> -% Copyright (C) 2015, 2017-2023 The Mercury team.
> +% Copyright (C) 2015, 2017-2024 The Mercury team.
>  % This file may only be copied under the terms of the GNU General
>  % Public License - see the file COPYING in the Mercury distribution.
>  %---------------------------------------------------------------------------%
> @@ -725,6 +725,8 @@ check_and_record_du_notag(TypeCtor, Context, Ctors, MaybeCanon,
>              MaybeDuRepn = have_errors([Spec])
>          ;
>              MaybeCanon = canon,
> +            % We can only get here for a non-subtype du type.

Do you think it would be helpful to include "non_sub" in the names
of the predicates that can only be reached from fill_in_non_sub_du_type_repn?
It would certainly make this code easier to check.

> @@ -3781,9 +3802,12 @@ search_ctor_repn_by_unqual_name([CtorRepn | CtorRepns], UnqualName, Arity,
>  
>  make_subtype_constructor_arg_repn(CtorArg, BaseCtorArgRepn, CtorArgRepn) :-
>      CtorArg = ctor_arg(MaybeFieldName, ArgType, Context),
> -    BaseCtorArgRepn = ctor_arg_repn(_MaybeBaseFieldName, _BaseArgType,
> -        ArgPosWidth, _BaseContext),
> -    CtorArgRepn = ctor_arg_repn(MaybeFieldName, ArgType, ArgPosWidth, Context).
> +    BaseCtorArgRepn = ctor_arg_repn(MaybeBaseFieldName, BaseMaybeBaseCtorArg,
> +        _BaseArgType, ArgPosWidth, _BaseContext),
> +    expect(unify(BaseMaybeBaseCtorArg, no_base_ctor_arg), $pred,
> +        "BaseMaybeBaseCtorArg != no_base_ctor_arg"),
> +    CtorArgRepn = ctor_arg_repn(MaybeFieldName,
> +        base_ctor_arg(MaybeBaseFieldName), ArgType, ArgPosWidth, Context).

I would add a pointer to this predicate in the comment above in the diff (which I deleted)
which says "this is filled in later".

> diff --git a/compiler/ml_code_util.m b/compiler/ml_code_util.m
> index 766854c38..83c705fec 100644
> --- a/compiler/ml_code_util.m
> +++ b/compiler/ml_code_util.m
> @@ -235,14 +235,21 @@
>  %
>  % Routines for dealing with fields.
>  %
> -
> -    % Given the user-specified field name, if any, and the argument number
> -    % (starting from one), generate an MLDS field name for the target language
> -    % type that represents the function symbol's cell when we are generating
> -    % code with --high-level-data.
> +    % ml_gen_hld_field_name(MaybeFieldName, MaybeBaseCtorArg, ArgNum) =
> +    %   FieldName:
>      %
> -:- func ml_gen_hld_field_name(maybe(ctor_field_name), int) =
> -    mlds_field_var_name.
> +    % Generate an MLDS field name for the target language type that represents
> +    % the function symbol's cell when we are generating code with
> +    % --high-level-data.
> +    %
> +    % MaybeFieldName is the user-specified field name (if any).
> +    % MaybeBaseCtorArg tells if this is a field in a subtype, and if so,

s/tells if/says whether/

> +    % the field name of the corresponding constructor argument in the base type
> +    % (if any).

Move the "(if any)" to just after "the field name".

> diff --git a/compiler/ml_unify_gen_util.m b/compiler/ml_unify_gen_util.m
> index a46b1435d..3dc626682 100644
> --- a/compiler/ml_unify_gen_util.m
> +++ b/compiler/ml_unify_gen_util.m
> @@ -319,7 +319,8 @@ allocate_consecutive_full_word_ctor_arg_repns_boxed(CurOffset,
>          [Var | Vars], [VarArgRepn | VarArgRepns]) :-
>      Type = ml_make_boxed_type,
>      ArgPosWidth = apw_full(arg_only_offset(CurOffset), cell_offset(CurOffset)),
> -    ArgRepn = ctor_arg_repn(no, Type, ArgPosWidth, dummy_context),
> +    ArgRepn = ctor_arg_repn(no, no_base_ctor_arg, Type, ArgPosWidth,
> +        dummy_context),
>      VarArgRepn = Var - ArgRepn,
>      allocate_consecutive_full_word_ctor_arg_repns_boxed(CurOffset + 1,
>          Vars, VarArgRepns).

I would change the name of this predicate, deleting "_boxed" and replacing it
with "_for_tuple". That would make clear that no_base_ctor_arg is justified here.

> @@ -333,7 +334,8 @@ allocate_consecutive_full_word_ctor_arg_repns_lookup(Info, CurOffset,
>          [Var | Vars], [VarArgRepn | VarArgRepns]) :-
>      ml_variable_type_direct(Info, Var, Type),
>      ArgPosWidth = apw_full(arg_only_offset(CurOffset), cell_offset(CurOffset)),
> -    ArgRepn = ctor_arg_repn(no, Type, ArgPosWidth, dummy_context),
> +    ArgRepn = ctor_arg_repn(no, no_base_ctor_arg, Type, ArgPosWidth,
> +        dummy_context),
>      VarArgRepn = Var - ArgRepn,
>      allocate_consecutive_full_word_ctor_arg_repns_lookup(Info, CurOffset + 1,
>          Vars, VarArgRepns).

I would consider something similar here, but I can't think of an expressive yet short suffix.

> diff --git a/compiler/mlds.m b/compiler/mlds.m
> index abb799a84..bf7d1da44 100644
> --- a/compiler/mlds.m
> +++ b/compiler/mlds.m
> @@ -2340,7 +2340,8 @@
>              % When compiling with --high-level-data, we generate a type
>              % in the target language for each data constructor in a
>              % discriminated union type. This is the name of one of the fields
> -            % of this type.
> +            % of this type. For a subtype, this has to be the name of a field
> +            % in the base type.

... of THE CORRESPONDING field in the base type.

The diff is otherwise fine.

Zoltan.