[m-rev.] for review: Fix bug with direct-arg ctors and intermodule optimisation.

[Zoltan Somogyi]

On Fri,  9 Jan 2026 17:25:57 +1100, Peter Wang <novalazy at gmail.com> wrote:

> If the compiler decides that a du type should use the direct-arg
> representation for some of its constructors, it must include information
> about that into the .opt file of the module defining the type, in the
> form of `where direct_arg is' clauses, which will be used by modules
> opt-importing that module and that type. That information was not being
> included for du types defined in the *interface* section of a module.
> 
> Also fix a related issue that was uncovered: a word_aligned_pointer
> assertion on a foreign_type definition would have no effect if there is
> a corresponding no-tag du type definition for the same type constructor.

"corresponding" is redundant, given "for the same constructor".

> compiler/hlds_data.m:
>     Add a helper predicate.

Lots of modules import hlds_data.m. Please see whether it makes sense
to add the new predicate to e.g. intermod_decide.m instead.

>  :- pred add_foreign_if_word_aligned_ptr(module_info::in, decide_du_params::in,
> -    type_ctor::in, foreign_type_body::in,
> +    type_ctor::in, foreign_type_body::in, bool::in,
>      component_type_map::in, component_type_map::out,
>      list(error_spec)::in, list(error_spec)::out) is det.
>  
>  add_foreign_if_word_aligned_ptr(ModuleInfo, Params, TypeCtor,
> -        ForeignType, !ComponentTypeMap, !Specs) :-
> +        ForeignType, ExpectForTarget, !ComponentTypeMap, !Specs) :-
>      DirectArgMap = Params ^ ddp_direct_arg_map,
>      ( if map.search(DirectArgMap, TypeCtor, _DirectArgFunctors) then
>          DirectArgPieces = [words("Error:"), qual_type_ctor(TypeCtor),
> @@ -2199,7 +2221,12 @@ add_foreign_if_word_aligned_ptr(ModuleInfo, Params, TypeCtor,
>              true
>          )
>      else
> -        unexpected($pred, "foreign type is not for this backend")
> +        (
> +            ExpectForTarget = yes,
> +            unexpected($pred, "foreign type is not for this backend")
> +        ;
> +            ExpectForTarget = no
> +        )
>      ).

I would replace the bool with a bespoke type, maybe with functors named
require_foreign_type_for_cur_backend and allow_foreign_type_for_any_backend.

> --- a/compiler/intermod.m
> +++ b/compiler/intermod.m
> @@ -2,7 +2,7 @@
>  % vim: ft=mercury ts=4 sw=4 et
>  %---------------------------------------------------------------------------%
>  % Copyright (C) 1996-2012 The University of Melbourne.
> -% Copyright (C) 2013-2025 The Mercury team.
> +% Copyright (C) 2013-2026 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.
>  %---------------------------------------------------------------------------%
> @@ -219,8 +219,16 @@ some_type_needs_to_be_written([], no).
>  some_type_needs_to_be_written([_ - TypeDefn | TypeCtorDefns], NeedWrite) :-
>      hlds_data.get_type_defn_status(TypeDefn, TypeStatus),
>      ( if
> -        ( TypeStatus = type_status(status_abstract_exported)
> -        ; TypeStatus = type_status(status_exported_to_submodules)
> +        (
> +            TypeStatus = type_status(status_exported),
> +            % A du type defined in the interface section (exported) will need
> +            % to be written to the .opt file if it has any direct-arg ctors.
> +            hlds_data.get_type_defn_body(TypeDefn, TypeBody),
> +            is_du_type_with_direct_arg_ctors(TypeBody)

I would extend either this comment, or the one in intermod_decide.m,
to say *why*; put the relevant part of the log message in front of the people
reading this. The other can be either a copy of the comment, or a pointer to
the other comment.

> --- a/tests/hard_coded/Mmakefile
> +++ b/tests/hard_coded/Mmakefile
> @@ -111,6 +111,7 @@ ORDINARY_PROGS = \
>  	det_in_semidet_cntxt \
>  	digraph_tc \
>  	dir_fold \
> +	direct_arg_opt_imported \
>  	direct_arg_partial_inst_1 \
>  	direct_arg_partial_inst_2 \
>  	direct_arg_tags_1 \

The new test case should be in a different list from ORDINARY_PROGS,
because the foreign code of some predicates exists only for C.
Please add it to C_ONLY_PROGS instead.

> diff --git a/tests/hard_coded/direct_arg_opt_imported.m b/tests/hard_coded/direct_arg_opt_imported.m
> new file mode 100644
> index 000000000..7bd3d299e
> --- /dev/null
> +++ b/tests/hard_coded/direct_arg_opt_imported.m
> @@ -0,0 +1,36 @@
> +%---------------------------------------------------------------------------%
> +% vim: ts=4 sw=4 et ft=mercury
> +%---------------------------------------------------------------------------%
> +%
> +% With intermodule optimisation enabled, the body of indirect_new_object is
> +% inlined into main/2, including the construction:
> +%
> +%   yes_object(Ob : object) : maybe_object

This talks about the particulars of this test.

> +% When a type is opt-imported (but not imported) from a module, we depend on
> +% information from the corresponding .opt file to tell us which of its
> +% constructors use the direct-arg representation.

This paragraph is generic. Rewrite to explain the specific applicability of this
point to this test case.

> +% The problem was that that additional information about the type was missing
> +% from the .opt file because maybe_object is defined in the interface section
> +% of its module, i.e. it was already exported.
> +%

Likewise.

> +:- implementation.
> +
> +:- import_module direct_arg_opt_imported_helper_1.
> +% not imported: direct_arg_opt_imported_helper_1.sub.

I would expand on this and say: the bug we are testing for occurred
only in the absence of the second import.

> diff --git a/tests/hard_coded/direct_arg_opt_imported_helper_1.sub.m b/tests/hard_coded/direct_arg_opt_imported_helper_1.sub.m

Since the big rename, we include "helper_N" in the names of submodules,
as shown e.g. by prince_frameopt_helper_1.prince_frameopt_helper_2.m.

This would be simpler if you shortened the name of the test, maybe to 
direct_arg_opt.

> +:- implementation.
> +
> +:- import_module list.
> +:- import_module string.
> +
> +:- pragma foreign_decl("C", "
> +typedef struct {
> +    MR_Word magic;
> +} Object;
> +").
> +
> +new_object(object(-1)).
> +
> +:- pragma foreign_proc("C",
> +    new_object(Ob::out),
> +    [will_not_call_mercury, thread_safe, promise_pure, may_not_export_body],
> +"
> +    Ob = MR_GC_NEW(Object);
> +    Ob->magic = 0xdeadbeef;
> +").
> +
> +maybe_check_object(MaybeOb, !IO) :-
> +    (
> +        MaybeOb = yes_object(Ob),
> +        get_magic(Ob, Magic),
> +        ( if Magic = 0xdeadbeef then
> +            io.write_string("ok\n", !IO)
> +        else if Magic = -1 then
> +            io.write_string("ok\n", !IO)
> +        else
> +            io.format("BUG: Magic = %#x\n", [i(Magic)], !IO)
> +        )
> +    ;
> +        MaybeOb = no_object
> +    ).
> +
> +:- pred get_magic(object::in, int::out) is det.
> +
> +get_magic(object(Magic), Magic).
> +
> +:- pragma foreign_proc(c,
> +    get_magic(Ob::in, Magic::out),
> +    [will_not_call_mercury, thread_safe, promise_pure, may_not_export_body],
> +"
> +    Magic = Ob->magic;
> +    assert(Magic == 0xdeadbeef);
> +").

I would put the definitions of new_object and get_magic next to
each other. I would also try to find a better name than "object"
for the data structure, though I can see that finding a better name
is not easy :-(

Other than the above, the diff is fine.

Zoltan.