[m-rev.] For Review: Refactor deep profiling module.

[Julien Fischer]

On Thu, 29 Nov 2007, Paul Bone wrote:

> This is currently running through the bootcheck for the deep profiling
> grade.  I have tested it by comparing HLDS dumps between this and a
> known good compiler.

It would also be a good idea to check that
`mdprof_test --verify-profile' succeeds with some profiles genererated
by the stage 2 compiler.

> Estimated hours taken: 3
> Branches: main
>
> Refactor the deep profiler module to remove duplicated code.  This
> change will make future changes to this module easier, many changes will
> only have to be done in one place rather than three.
>
> compiler/deep_profiling.m:
> 	+ Factored out common code between the transformations for det, semidet
>      and nondet procedures.
> 	+ Replaced cases of 2-4 repeated 'create a variable' conjuncts with a
> 	  single call to generate_var.




> Index: compiler/deep_profiling.m
> ===================================================================
> RCS file: /home/mercury/mercury1/repository/mercury/compiler/deep_profiling.m,v
> retrieving revision 1.68
> diff -u -r1.68 deep_profiling.m
> --- compiler/deep_profiling.m	23 Nov 2007 07:34:58 -0000	1.68
> +++ compiler/deep_profiling.m	29 Nov 2007 10:49:37 -0000
> @@ -486,7 +486,8 @@
>         ->
>             transform_inner_proc(ModuleInfo, PredProcId, !ProcInfo)
>         ;
> -            transform_det_proc(ModuleInfo, PredProcId, !ProcInfo)
> +            transform_proc(transform_proc_det, ModuleInfo, PredProcId,
> +                !ProcInfo)
>         )
>     ;
>         CodeModel = model_semi,
> @@ -498,11 +499,13 @@
>         ->
>             transform_inner_proc(ModuleInfo, PredProcId, !ProcInfo)
>         ;
> -            transform_semi_proc(ModuleInfo, PredProcId, !ProcInfo)
> +            transform_proc(transform_proc_semi, ModuleInfo, PredProcId,
> +                !ProcInfo)
>         )
>     ;
>         CodeModel = model_non,
> -        transform_non_proc(ModuleInfo, PredProcId, !ProcInfo)
> +        transform_proc(transform_proc_non, ModuleInfo, PredProcId,
> +            !ProcInfo)
>     ).
>
> %-----------------------------------------------------------------------------%
> @@ -514,41 +517,39 @@
>                 deep_current_csd        :: prog_var,
>                 deep_site_num_counter   :: counter,
>                 deep_call_sites         :: list(call_site_static_data),
> -                deep_varset             :: prog_varset,
> -                deep_var_types          :: vartypes,
> +                deep_varinfos               :: var_info,
>                 deep_proc_filename      :: string,
>                 deep_maybe_rec_info     :: maybe(deep_recursion_info)
>             ).
>
> -:- pred transform_det_proc(module_info::in, pred_proc_id::in,
> -    proc_info::in, proc_info::out) is det.
>
> -transform_det_proc(ModuleInfo, PredProcId, !ProcInfo) :-
> +:- type transform_proc_type
> +    --->    transform_proc_det
> +    ;       transform_proc_semi
> +    ;       transform_proc_non.

Document this type.  Why is it even necessary, can't you just use the
code model as the first argument of transform_proc/5?

> +% Transfrom a procedure.  The first argument is model specific, and is used to
> +% wrap the transformed goal in port code.
> +%

The indentation of that comment looks wrong (perhaps it's just my mail
reader?)

> +:- pred transform_proc(transform_proc_type::in, module_info::in,
> +    pred_proc_id::in, proc_info::in, proc_info::out) is det.
> +
> +transform_proc(TransformType, ModuleInfo, PredProcId, !ProcInfo) :-
>     proc_info_get_goal(!.ProcInfo, Goal0),
>     Goal0 = hlds_goal(_, GoalInfo0),
> -    some [!VarSet, !VarTypes] (
> -        proc_info_get_varset(!.ProcInfo, !:VarSet),
> -        proc_info_get_vartypes(!.ProcInfo, !:VarTypes),
> -        CPointerType = c_pointer_type,
> -        svvarset.new_named_var("TopCSD", TopCSD, !VarSet),
> -        svvarset.new_named_var("MiddleCSD", MiddleCSD, !VarSet),
> -        svvarset.new_named_var("ProcStaticLayout", ProcStaticVar, !VarSet),
> -        svmap.set(TopCSD, CPointerType, !VarTypes),
> -        svmap.set(MiddleCSD, CPointerType, !VarTypes),
> -        svmap.set(ProcStaticVar, CPointerType, !VarTypes),
> -        module_info_get_globals(ModuleInfo, Globals),
> -        globals.lookup_bool_option(Globals, use_activation_counts,
> -            UseActivationCounts),
> -        (
> -            UseActivationCounts = no,
> -            svvarset.new_named_var("ActivationPtr", ActivationPtr0, !VarSet),
> -            svmap.set(ActivationPtr0, CPointerType, !VarTypes),
> -            MaybeActivationPtr = yes(ActivationPtr0)
> -        ;
> -            UseActivationCounts = yes,
> -            MaybeActivationPtr = no
> -        ),
> -        ExcpVars = hlds_deep_excp_vars(TopCSD, MiddleCSD, MaybeActivationPtr),
> +    proc_info_get_varset(!.ProcInfo, VarSet0),
> +    proc_info_get_vartypes(!.ProcInfo, VarTypes0),
> +    some [!VarInfo] (
> +        !:VarInfo = var_info(VarSet0, VarTypes0),
> +        generate_var("TopCSD", c_pointer_type, TopCSD, !VarInfo),
> +        generate_var("MiddleCSD", c_pointer_type, MiddleCSD, !VarInfo),
> +        generate_var("ProcStaticLayout", c_pointer_type, ProcStaticVar,
> +            !VarInfo),
> +
>         proc_info_get_context(!.ProcInfo, Context),
>         FileName = term.context_file(Context),
>         LineNumber = term.context_line(Context),
> @@ -556,32 +557,76 @@
>         proc_info_get_maybe_deep_profile_info(!.ProcInfo, MaybeDeepProfInfo),
>         extract_deep_rec_info(MaybeDeepProfInfo, MaybeRecInfo),
>         DeepInfo0 = deep_info(ModuleInfo, PredProcId, MiddleCSD,
> -            counter.init(0), [], !.VarSet, !.VarTypes, FileName, MaybeRecInfo)
> -    ),
> +            counter.init(0), [], !.VarInfo, FileName, MaybeRecInfo),
>
> -    deep_prof_transform_goal(empty, Goal0, TransformedGoal, _,
> -        DeepInfo0, DeepInfo),
> +        % This call transforms the goals of the procedure.
> +        deep_prof_transform_goal(empty, Goal0, TransformedGoal, _,
> +            DeepInfo0, DeepInfo),
>
> -    Vars = DeepInfo ^ deep_varset,
> -    VarTypes = DeepInfo ^ deep_var_types,
> -    CallSites = DeepInfo ^ deep_call_sites,
> +        !:VarInfo = DeepInfo ^ deep_varinfos,
> +        CallSites = DeepInfo ^ deep_call_sites,
>
> -    (
> -        MaybeRecInfo = yes(RecInfo),
> -        RecInfo ^ role = inner_proc(OuterPredProcId)
> -    ->
> -        OuterPredProcId = proc(PredId, ProcId)
> -    ;
> -        PredProcId = proc(PredId, ProcId)
> +        (
> +            MaybeRecInfo = yes(RecInfo),
> +            RecInfo ^ role = inner_proc(OuterPredProcId)
> +        ->
> +            OuterPredProcId = proc(PredId, ProcId)
> +        ;
> +            PredProcId = proc(PredId, ProcId)
> +        ),
> +
> +        module_info_get_globals(ModuleInfo, Globals),
> +        globals.lookup_bool_option(Globals, use_activation_counts,
> +            UseActivationCounts),
> +
> +        IsInInterface = is_proc_in_interface(ModuleInfo, PredId, ProcId),
> +        ProcStatic = hlds_proc_static(FileName, LineNumber, IsInInterface,
> +            CallSites),
> +        ShroudedPredProcId = shroud_pred_proc_id(proc(PredId, ProcId)),
> +        ProcStaticConsId = deep_profiling_proc_layout(ShroudedPredProcId),
> +        generate_unify(ProcStaticConsId, ProcStaticVar, BindProcStaticVarGoal),
> +
> +        % Wrap goal body inside more goals that run the port code as nessacary.

(s/nessacary/necessary/)


Wrap the procedure body inside ...

...

> @@ -1435,9 +1350,8 @@
>
>     SiteNumCounter0 = !.DeepInfo ^ deep_site_num_counter,
>     counter.allocate(SiteNum, SiteNumCounter0, SiteNumCounter),
> -    varset.new_named_var(!.DeepInfo ^ deep_varset, "SiteNum", SiteNumVar,
> -        VarSet),
> -    map.set(!.DeepInfo ^ deep_var_types, SiteNumVar, int_type, VarTypes),
> +    generate_var("SiteNum", int_type, SiteNumVar, !.DeepInfo ^ deep_varinfos,
> +        VarInfo),
>     generate_unify(int_const(SiteNum), SiteNumVar, SiteNumVarGoal),
>
>     generate_deep_det_call(ModuleInfo, "prepare_for_callback", 1,
> @@ -1453,8 +1367,7 @@
>     Goal = hlds_goal(conj(plain_conj, [SiteNumVarGoal, PrepareGoal, Goal0]),
>         GoalInfo),
>     !:DeepInfo = !.DeepInfo ^ deep_site_num_counter := SiteNumCounter,
> -    !:DeepInfo = !.DeepInfo ^ deep_varset := VarSet,
> -    !:DeepInfo = !.DeepInfo ^ deep_var_types := VarTypes,
> +    !:DeepInfo = !.DeepInfo ^ deep_varinfos := VarInfo,
>     !:DeepInfo = !.DeepInfo ^ deep_call_sites :=
>         !.DeepInfo ^ deep_call_sites ++ [CallSite].
>

...

> @@ -1726,6 +1630,72 @@
>         unify_context(umc_explicit, [])),
>     Goal = hlds_goal(GoalExpr, GoalInfo).
>
> +
> +:- type var_info
> +    --->    var_info(
> +                varinfo_varset      :: prog_varset,
> +                varinfo_vartypes    :: vartypes).



The final parenthesis should line up with the beginning of var_info,
e.g.

 	:- type var_info
 		--->	var_info(
 			...
 			)

Document this type.  I also suggest moving its definition to just below
that of the deep_info/0 type.

> +:- pred generate_var(string::in, mer_type::in, prog_var::out, var_info::in,
> +    var_info::out) is det.
> +
> +generate_var(Name, Type, Var, !VarInfo) :-
> +    some [!VarSet, !VarTypes]
> +    (
> +        !.VarInfo = var_info(!:VarSet, !:VarTypes),
> +        generate_var(Name, Type, Var, !VarSet, !VarTypes),
> +        !:VarInfo = var_info(!.VarSet, !.VarTypes)
> +    ).
> +
> +
> +:- pred generate_var(string::in, mer_type::in, prog_var::out, prog_varset::in,
> +    prog_varset::out, vartypes::in, vartypes::out) is det.
> +
> +generate_var(Name, Type, Var, !VarSet, !VarTypes) :-
> +    svvarset.new_named_var(Name, Var, !VarSet),
> +    svmap.set(Var, Type, !VarTypes).

I suggest naming this predicate, generate_var_2, in order to distinguish
it from the similarly named predicate above.

Julien.
--------------------------------------------------------------------------
mercury-reviews mailing list
Post messages to:       mercury-reviews at csse.unimelb.edu.au
Administrative Queries: owner-mercury-reviews at csse.unimelb.edu.au
Subscriptions:          mercury-reviews-request at csse.unimelb.edu.au
--------------------------------------------------------------------------