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

[Paul Bone]

On Fri, Nov 30, 2007 at 03:53:38PM +1100, Julien Fischer wrote:
> 
> 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.
> 

Will do.  It's currently running through the bootcheck ATM.

> 
> >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?
> 

This is now removed.  transform_proc takes one fewer argument and the 
code above is also much simpler.  (This seems obvious now, doh!)


> >+% 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?)
>

Fixed.

> >+
> >+        % Wrap goal body inside more goals that run the port code as 
> >nessacary.
> 
> (s/nessacary/necessary/)
>

Fixed.

> 
> 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].
> >
> 
> ...
> 

I don't understand.  What does ... mean?


> >@@ -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.
>

Fixed these three things.


> >+:- 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.
>

Done.

I won't commit until I understand your suggestion above regarding '...'.
And I'll test the deep profile output files too.

Thanks.


--------------------------------------------------------------------------
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
--------------------------------------------------------------------------