[m-rev.] fore post-commit review: improve the speed of the typechecker

[Paul Bone]

On Fri, Jul 30, 2010 at 03:14:21PM +1000, Zoltan Somogyi wrote:
> For review by anyone. The only non-trivial change is the one to typechecker.m.
> 
> Index: compiler/pred_table.m
> ===================================================================
> RCS file: /home/mercury/mercury1/repository/mercury/compiler/pred_table.m,v
> retrieving revision 1.14
> diff -u -b -r1.14 pred_table.m
> --- compiler/pred_table.m	30 Mar 2010 23:57:25 -0000	1.14
> +++ compiler/pred_table.m	20 Jul 2010 04:02:39 -0000
> @@ -88,11 +88,22 @@
>      predicate_table::in, predicate_table::out) is det.
>  
>      % Get a list of all the valid predids in the predicate_table.
> +    % (Predicates whose definition contains a type error, etc.
> +    % get removed from this list, so that later passes can rely
> +    % on the predicates in this list being type-correct, etc.)
>      %
>      % This operation does not logically change the predicate table,
>      % but does update it physically.
>      %
> -:- pred predicate_table_get_predids(list(pred_id)::out,
> +:- pred predicate_table_get_valid_predids(list(pred_id)::out,
> +    predicate_table::in, predicate_table::out) is det.
> +
> +    % Set the list of the pred_ids of all the valid predicates.
> +    % NOTE: The only approved way to specify the list is to call
> +    % module_info_get_valid_predids or predicate_table_get_valid_predids,
> +    % and remove some pred_ids from that list.
> +    %
> +:- pred predicate_table_set_valid_predids(list(pred_id)::in,
>      predicate_table::in, predicate_table::out) is det.
>  
>      % Remove a pred_id from the valid list.

I misunderstood this comment the first time I read it.  I thought it meant that
I shouldn't ever call this predicate.  To clarify: if I want to remove a
predicate from the valid predids to do I:

    predicate_table_get_valid_predids(List0, !Table),
    filter(..., List0, List),
    predicate_table_set_valid_predids(List, !Table)

If so could you add "... and then use this call to set the list.".

> Index: compiler/typecheck.m

typecheck.m is fine.  I know I commented on the trivial change and not the
significant one, but the significant change needed no comment.

> Index: library/assoc_list.m
> ===================================================================
> RCS file: /home/mercury/mercury1/repository/mercury/library/assoc_list.m,v
> retrieving revision 1.26
> diff -u -b -r1.26 assoc_list.m
> --- library/assoc_list.m	25 Mar 2010 01:28:53 -0000	1.26
> +++ library/assoc_list.m	29 Jul 2010 04:25:29 -0000
> @@ -99,6 +99,42 @@
>  :- func assoc_list.map_values(func(K, V) = W, assoc_list(K, V))
>      = assoc_list(K, W).
>  
> +    % assoc_list.filter(Pred, List, TrueList) takes a closure with one
> +    % input argument and for each member K - V of List X, calls the closure
> +    % on the key. K - V is included in TrueList iff Pred(K) is true.
> +    %
> +:- pred assoc_list.filter(pred(K)::in(pred(in) is semidet),
> +    assoc_list(K, V)::in, assoc_list(K, V)::out) is det.
> +:- func assoc_list.filter(pred(K)::in(pred(in) is semidet),
> +    assoc_list(K, V)::in) = (assoc_list(K, V)::out) is det.

Should this be called filter_by_key, and below.

> +    % assoc_list.negated_filter(Pred, List, FalseList) takes a closure with one
> +    % input argument and for each member K - V of List X, calls the closure
> +    % on the key. K - V is included in FalseList iff Pred(K) is false.
> +    %
> +:- pred assoc_list.negated_filter(pred(K)::in(pred(in) is semidet),
> +    assoc_list(K, V)::in, assoc_list(K, V)::out) is det.
> +:- func assoc_list.negated_filter(pred(K)::in(pred(in) is semidet),
> +    assoc_list(K, V)::in) = (assoc_list(K, V)::out) is det.
> +
> +    % assoc_list.filter(Pred, List, FalseList) takes a closure with one
> +    % input argument and for each member K - V of List X, calls the closure
> +    % on the key. K - V is included in TrueList iff Pred(K) is true.
> +    % K - V is included in FalseList iff Pred(K) is false.
> +    %
> +:- pred assoc_list.filter(pred(K)::in(pred(in) is semidet),
> +    assoc_list(K, V)::in, assoc_list(K, V)::out, assoc_list(K, V)::out) is det.

And here.

> +    % assoc_list.merge(L1, L2, L):
> +    %
> +    % L is the result of merging the elements of L1 and L2, in ascending order.
> +    % L1 and L2 must be sorted on the keys.
> +    %

Is L sorted also?

> +assoc_list.merge(As, Bs) = ABs :-
> +    assoc_list.merge(As, Bs, ABs).
> +
> +assoc_list.merge([], [], []).
> +assoc_list.merge([A | As], [], [A | As]).
> +assoc_list.merge([], [B | Bs], [B | Bs]).
> +assoc_list.merge([A | As], [B | Bs], Cs) :-
> +    (
> +        A = AK - _AV,
> +        B = BK - _BV,

You can put these two deconstructions outside of the ITE, the compiler will
assert that they are deterministic.

> +        compare(>, AK, BK)
> +    ->
> +        assoc_list.merge([A | As], Bs, Cs0),
> +        Cs = [B | Cs0]
> +    ;
> +        % If compare((=), AK, BK), take A first.
> +        assoc_list.merge(As, [B | Bs], Cs0),
> +        Cs = [A | Cs0]
>      ).
> Index: tools/speedtest
> ===================================================================
> RCS file: /home/mercury/mercury1/repository/mercury/tools/speedtest,v
> retrieving revision 1.20
> diff -u -b -r1.20 speedtest
> --- tools/speedtest	4 Sep 2009 02:50:15 -0000	1.20
> +++ tools/speedtest	29 Jul 2010 10:54:02 -0000
> @@ -2,15 +2,20 @@
>  #
>  # A program to test different versions of the compiler.
>  
> -usage="Usage: speedtest [-c cmd] [-dns] batchname"
> -limit=6
> -cmd_llds="mmc -C -O2 --grade asm_fast.gc llds_out.m typecheck.m mercury_compile.m modules.m code_info.m polymorphism.m"
> -cmd_mlds="mmc -C -O2 --grade hlc.gc llds_out.m typecheck.m mercury_compile.m modules.m code_info.m polymorphism.m"
> -cmd="$cmd_llds"
> +usage="Usage: speedtest [-dhlstz] [-c cmd] [-nN] [-ON] [-fFILE] batchname"
> +
> +short_modulelist="llds_out.m typecheck.m mercury_compile.m modules.m code_info.m polymorphism.m"
> +long_modulelist="$short_modulelist options.m add_pragma.m simplify.m table_gen.m mlds_to_java.m mlds_to_il.m"
> +

That's a nice improvement, Thanks.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 489 bytes
Desc: Digital signature
URL: <http://lists.mercurylang.org/archives/reviews/attachments/20100730/9e00760c/attachment.sig>