[m-rev.] for review: build HLDS by item kind

[Julien Fischer]

Hi Zoltan,

On Thu, 29 Oct 2015, Zoltan Somogyi wrote:

> scripts/mtags.in

Delete that.

> Add items to the HLDS by kinds, not in passes.
> 
> There are several reasons for this change.
> 
> First, it is more flexible than the previous arrangement of adding items
> to the HLDS in three passes. Since adding a fourth pass would have had
> significant performance implications, we refrained from making changes
> that would have required a fourth pass in a pass-based algorithm.
> With the new structure, such changes do not lead to any detectable slowdown,
> so there is no reason why we should avoid making them.
> 
> In fact, the immediate motivation for this diff is to make such a change.
> This is a fix for Mantis bug 318, which requires adding abstract and/or
> non-foreign definitions for types because foreign definitions for those types
> *regardless* of their relative order in the source code.

That last sentence doesn't make sense.

> This diff also has the side effect that e.g. the mode declarations for
> a predicate don't have to follow the declaration of the (type of) the predicate
> itself. Pred and mode declarations used to be processed in the same pass,
> but now, all pred declaration are processed before all mode declarations.
> 
> Second, as identified earlier, the new structure is conceptually cleaner than
> the old one, and the constraints on when different kinds of items have to be
> added to the HLDS can documented next to the code that does the adding,
> since that code is no longer mixed with code that adds other kinds of items
> at the same time.
> 
> Third, the code we used to use to detect invalid type definitions
> was only a crude approximation. With the new setup, it is trivial to use
> an exact computation. We now proceed to later compiler passes in many cases
> that previously led us to stop before type checking because we (erroneously)
> believed that the program had an invalid type definition.
> 
> Fourth, my benchmarking shows a speedup on tools/speedtest -l -m of about
> 1.2% to 1.5%. The new approach does allocate a bit more memory (a pair
> and a cons cell per item), but it traverses most items just once, not
> three times. A few kinds of items (e.g. pred declarations) do have to be
> processed more than once, e.g. both before and after some other kinds
> of items are added to the HLDS, but four of the five most frequently items

s/frequently/frequent/

> in bootchecks (the pragmas recording the results of the termination and
> exception analyses, mode declarations, and clauses) are processed just once.
> Evidently, the speedup from traversing fewer items, and avoiding the
> switches on item kinds that those traversals involve, is greater than
> the extra cost of the additional memory allocations, including their effect
> on garbage collectin times.

s/collectin/collection/

...

> compiler/check_typeclass.m:
>     Improve an error message. We now get this error message even for modules
>     on which the compiler previously stopped before getting to typechecking.
> 
> compiler/modes.m:
> compiler/prog_item_stats.m:

The description of the changes to these files is missing.

...


> diff --git a/compiler/add_pragma.m b/compiler/add_pragma.m
> index e871b09..1c23133 100644
> --- a/compiler/add_pragma.m
> +++ b/compiler/add_pragma.m
> @@ -20,13 +20,59 @@
>
>  %-----------------------------------------------------------------------------%
> 
> -:- pred add_pass_2_pragma(item_pragma_info::in,
> -    item_mercury_status::in, module_info::in, module_info::out,
> +:- inst item_pragma_info(I) for item_pragma_info/0
> +    --->    item_pragma_info(I, ground, ground, ground).
> +
> +:- inst pragma_pass_2 for pragma_type/0

If you are going to start using typed insts in the compiler, then you should
update the configure script's up-to-date check to ensure that they are supported
by a bootstrap compiler.
...

> diff --git a/compiler/add_type.m b/compiler/add_type.m
> index 3fe91aa..a239524 100644
> --- a/compiler/add_type.m
> +++ b/compiler/add_type.m
> @@ -439,6 +439,17 @@ process_type_defn(TypeCtor, TypeDefn, !FoundInvalidType, !ModuleInfo,
>      ),
>      (
>          !.FoundInvalidType = found_invalid_type
> +        % If there is an error in this type definition, we may not
> +        % be able to add the special speds for it correctly.

s/speds/preds/


> +        % Even errors occurred only in other types, adding the special

s/Even errors/Even if the errors/

> +        % predicates for this error-free type wouldn't help us, because
> +        % the presence of an invalid type anywhere will prevent the compiler
> +        % from going on to process those special predicates. It won't even
> +        % look at them to find errors, such as user-defined unify and compare
> +        % predicates being not found or having the wrong signature.
> +        % XXX If this ever changes, we *should* add the special preds
> +        % for error-free type definitions regardless of the presence
> +        % of any previous type errors.
>      ;
>          !.FoundInvalidType = did_not_find_invalid_type,
>          % XXX kind inference:

...

> diff --git a/compiler/make_hlds_passes.m b/compiler/make_hlds_passes.m
> index f5fcf50..3c2d91e 100644
> --- a/compiler/make_hlds_passes.m
> +++ b/compiler/make_hlds_passes.m

...

> +    % The second part of the old pass 1.
> +
> +    % We process inst definitions after all type definitions because
> +    % the processing of type-specific insts may require access to the
> +    % definition of any type constructor.
> +    list.foldl3(add_inst_defn, ItemInstDefns,
> +        !ModuleInfo, !FoundInvalidInstOrMode, !Specs),
> +
> +    % Mode definitions may refer to user defined insts. We don't yet
> +    % exploit the availability of all inst definitions when we process
> +    % a mode definition, but in the future, we could.
> +    list.foldl3(add_mode_defn, ItemModeDefns,
> +        !ModuleInfo, !FoundInvalidInstOrMode, !Specs),
> +
> +    % A predicate declaration defines the type of the arguments of a predicate,
> +    % and may give the modes of those arguments. We don't yet exploit
> +    % the availability of all type and mode definitions when we process
> +    % a pred declaration (e.g. for error checking), but in the future,
> +    % we could.
> +    list.foldl2(add_pred_decl, ItemPredDecls,
> +        !ModuleInfo, !Specs),
> +
> +    % We need to process the mode declaration of a predicate
> +    % after we have seen the (type) declaration of the predicate.
> +    % In the past, this required the predicate declaration to precede
> +    % the mode declaration in the source code.
> +    list.foldl2(add_mode_decl, ItemModeDecls,
> +        !ModuleInfo, !Specs),
> +    list.foldl2(add_solver_type_aux_pred_decls, SolverAuxPredInfos,
> +        !ModuleInfo, !Specs),
> +
> +    % Every mutable has its own set of access predicates. Declare them,
> +    % whether the mutable definition was a standalone item or part of
> +    % the definition of a solver type.
> +    list.foldl2(add_aux_pred_decls_for_mutable_if_local, ItemMutables,
> +        !ModuleInfo, !Specs),
> +    list.foldl2(add_aux_pred_decls_for_mutable_if_local, SolverItemMutables,
> +        !ModuleInfo, !Specs),
> +
> +    % Record typeclass definitions.
> +    list.foldl2(add_typeclass_defn, ItemTypeclasses,
> +        !ModuleInfo, !Specs),
> +
> +    % The old pass 2.
> +
> +    % Now that we have processed all mode declarations, we have a reliable
> +    % record of which declarated predicates and functions have NO mode

s/declarated/declared/

> +    % declaration. For functions, this is not an error, since Mercury
> +    % specifies a implicit mode declaration for them. maybe_add_default_mode
> +    % add that implicit mode declaration.
> +    % XXX It should also generat error messages for PREDICATES that have

s/generat/generate/

The change looks fine otherwise.

Julien.