[m-rev.] For post-commit review: Coverage profiler no-longer instruments deep profiler instrumentation goals.
Paul Bone
pbone at csse.unimelb.edu.au
Wed Sep 17 10:41:01 AEST 2008
On Tue, Sep 16, 2008 at 06:57:54PM +1000, Zoltan Somogyi wrote:
> Index: compiler/deep_profiling.m
> ===================================================================
> RCS file: /home/mercury/mercury1/repository/mercury/compiler/deep_profiling.m,v
> retrieving revision 1.81
> diff -u -b -r1.81 deep_profiling.m
> --- compiler/deep_profiling.m 14 Sep 2008 03:52:53 -0000 1.81
> +++ compiler/deep_profiling.m 16 Sep 2008 08:54:13 -0000
> @@ -2062,44 +2074,40 @@
> % Consider inserting a coverage point after this goal to measure how
> % many solutions it may have.
> (
> - IsMDProfInst = goal_is_not_mdprof_inst,
> - (
> - GoalTrivial = goal_is_nontrivial,
> - (
> - GoalHasPortCounts = goal_does_not_have_port_counts,
> - (
> - CoverageAfterKnown0 = coverage_after_unknown,
> -
> (
> - % I havn't thought about parallel conjunctions here,
> - % They should have the same declartive semantics as
> - % plain conjunctions, and I beleive they arn't possible
> - % where the first goal is anything other than
> - % model_det, Only other models can affect the coverage
> - % after the second goal. However I can't be sure the
> - % implementation won't change.
> - %
> - % Because parallel conjunctions arn't common and this
> - % is really only an optimiztion so I've only enabled it
> - % for plain conjunctions.
> -
> + % Never insert coverage points on goals that are part of the deep
> + % profiling instrumentation.
> + IsMDProfInst = goal_is_mdprof_inst
> + ;
> + % The goal itself has the counter we need; adding a second counter
> + % would be redundant.
> + GoalHasPortCounts = goal_has_port_counts
> + ;
> + % We already have execution counts for the program point after this
> + % goal; adding a counter would be redundant.
> + CoverageAfterKnown0 = coverage_after_known
> + ;
> + % We don't need to know the execution count of this goal, since
> + % it is too cheap to matter.
> + GoalTrivial = goal_is_trivial
> + ;
> + % Never insert coverage points after conjunctions; wait until
> + % the algorithm recurses to inside the conjunction and make a
> + % better decision about the last conjunct. This can reduce the
> + % number of coverage points inserted in some cases.
> GoalExpr0 = conj(plain_conj, _)
> - ->
> - % Never insert coverage points after conjunctions, wait
> - % until the algorithm recurses to inside the
> - % conjunction and make a better decision about the last
> - % conjunct. This can reduce the number of coverage
> - % points inserted in some cases.
>
> + % We haven't yet thought about whether or not we should treat
> + % parallel conjunctions the same way.
> + )
> + ->
> MaybeCPType = no
> ;
> (
> ( Detism = detism_semi
> ; Detism = detism_cc_non
> ),
> -
> - CoverMayFail = !.Info ^ ci_coverage_profiling_opts ^
> - cpo_may_fail,
> + CoverMayFail = !.Info ^ ci_coverage_profiling_opts ^ cpo_may_fail,
> (
> CoverMayFail = yes,
> MaybeCPType = yes(cp_type_solns_may_fail)
> @@ -2109,9 +2117,7 @@
> )
> ;
> Detism = detism_multi,
> -
> - CoverMulti = !.Info ^ ci_coverage_profiling_opts ^
> - cpo_multi,
> + CoverMulti = !.Info ^ ci_coverage_profiling_opts ^ cpo_multi,
> (
> CoverMulti = yes,
> MaybeCPType = yes(cp_type_solns_multi)
> @@ -2121,9 +2127,7 @@
> )
> ;
> Detism = detism_non,
> -
> - CoverAny = !.Info ^ ci_coverage_profiling_opts ^
> - cpo_any,
> + CoverAny = !.Info ^ ci_coverage_profiling_opts ^ cpo_any,
> (
> CoverAny = yes,
> MaybeCPType = yes(cp_type_solns_any)
> @@ -2132,42 +2136,23 @@
> MaybeCPType = no
> )
> ;
> - % In this case we know that execution will always stop
> - % at this goal, no coverage point is needed (or would
> - % work).
> ( Detism = detism_erroneous
> ; Detism = detism_failure
> ),
> + % Execution cannot reach the point after this goal, so we need
> + % no coverage point.
> MaybeCPType = no
> ;
> - % This should never occur, as other coverage points
> - % would have been inserted to ensure coverage is known
> - % here, unless they are disabled. We don't insert a
> - % coverage point here since we shouldn't have to.
> ( Detism = detism_det
> ; Detism = detism_cc_multi
> ),
> + % This should never occur, as coverage points should already have
> + % been inserted to ensure coverage is known here, unless the
> + % relevant kind of coverage points are disabled. We don't insert
> + % a coverage point here since we shouldn't have to.
> + % XXX In that case, why don't we call unexpected() here? -zs
> MaybeCPType = no
After looking at this carefully, it's quite normal that this will occur.
I've updated the comment.
> )
> - )
> - ;
> - CoverageAfterKnown0 = coverage_after_known,
> - MaybeCPType = no
> - )
> - ;
> - GoalHasPortCounts = goal_has_port_counts,
> - MaybeCPType = no
> - )
> - ;
> - GoalTrivial = goal_is_trivial,
> - MaybeCPType = no
> - )
> - ;
> - % Never insert coverage points on goals that are part of the deep
> - % profiling instrumentation.
> - %
> - IsMDProfInst = goal_is_mdprof_inst,
> - MaybeCPType = no
> ),
>
> % Step 2.
Thanks for removing the deep indenting here, this is much clearer.
> @@ -2551,19 +2522,12 @@
> % beginning of each branch,
>
> CPOBranchIf = !.Info ^ ci_coverage_profiling_opts ^ cpo_branch_ite,
> - CondMaybeDPCoverageInfo = goal_info_get_maybe_dp_coverage_info(
> - Cond0 ^ hlds_goal_info),
> - (
> - CondMaybeDPCoverageInfo =
> - yes(dp_coverage_goal_info(_, CondHasPortCounts))
> - ;
> - CondMaybeDPCoverageInfo = no,
> - CondHasPortCounts = goal_does_not_have_port_counts
> - ),
> + CondHasPortCounts = goal_get_maybe_dp_has_port_counts(Cond0),
> DPInfo = dp_goal_info(IsMDProfInst, _),
> (
> CPOBranchIf = yes,
> CondHasPortCounts = goal_does_not_have_port_counts,
> + % XXX This test looks to be a bug; I think it should be reversed. -zs
> IsMDProfInst = goal_is_mdprof_inst
> ->
> (
Yes, this is a bug. I've fixed it.
> @@ -2700,6 +2670,8 @@
> % this is at the beginning of the branch since it's used for the main
> % pass which is done in reverse.
> %
> + % XXX That documentation does not make sense.
> + %
> :- pred coverage_after_known_branch(coverage_after_known::in,
> coverage_after_known::in, coverage_after_known::out) is det.
>
I've re-written this comment, it's now much clearer.
> @@ -2796,14 +2769,14 @@
> % Used to gather some information about goals before the coverage
> % transformation.
> %
> + % XXX Document *what* information this pass gathers.
> + %
> :- pred coverage_prof_first_pass(coverage_profiling_options::in, hlds_goal::in,
> hlds_goal::out, goal_has_port_counts::in, dp_coverage_goal_info::out)
> is det.
Fixed.
> @@ -2814,6 +2787,9 @@
> % naive assumption in the second pass is fixed (even when this first
> % pass is not used).
> %
> + % XXX I don't understand this comment. Are you talking about builtin
> + % calls? Calls to the profiling primitives themselves? What is the
> + % naive assumption, and why is it a problem? -zs
> ( GoalExpr0 = plain_call(_, _, _, _, _, _)
> ; GoalExpr0 = generic_call(_, _, _, _)
> ; GoalExpr0 = call_foreign_proc(_, _, _, _, _, _, _)
I've clarified this.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mercurylang.org/archives/reviews/attachments/20080917/4d59cdb3/attachment.sig>
More information about the reviews
mailing list