[m-dev.] for review: --dead-code

[Zoltan Somogyi]

On 20-Jul-2000, Fergus Henderson <fjh at cs.mu.OZ.AU> wrote:
> This transformation is more like a static version of lazy evaluation.
> Perhaps `--semi-lazy-code'?

I don't like that name; in most ways this operation has nothing to do with
lazyness. How about --eliminate-redundancies? Or --redundant-code?

> One potential problem with this optimization as written is that it can
> increase code size, by duplicating the same goal in more than one
> branch.  It would be a good idea to have a version of it which only
> did the transformation if the code was used in at most one branch.
> This would be particularly appropriate in the case when `--opt-space'
> is enabled.

I can handle this by introducing a new integer option, perhaps called
--eliminate-redundancies-copy-limit, and not copying a goal to more places
than the value of this option.

> It might also be useful to have a version which only does the
> transformation if the code is used in zero branches, i.e. only does
> code elimination rather than moving code;

--eliminate-redundancies-copy-limit 0 would achieve this.

> that version could be
> allowed for the case where the user specifies `--no-fully-strict
> --no-reorder-conj', whereas the other versions would require
> both `--no-fully-strict' and `--reorder-conj'.

This can be done by having --no-reorder-conj implying
--eliminate-redundancies-copy-limit 0.

> (If there is some reason why the documentation should not be
> exposed to users yet, the documentation should still be added,
> commented out, together with an explanation of why it is commented out.)

The reason is the slowdown in tree234, as I said at the meeting.

> > +dead_code__process_proc_msg(PredId, ProcId, ProcInfo0, ProcInfo,
> > +		ModuleInfo0, ModuleInfo) -->
> > +	globals__io_lookup_bool_option(very_verbose, VeryVerbose),
> > +	globals__io_lookup_int_option(vn_fudge, Levels),
> 
> This undocumented `vn_fudge' option is very mysterious,
> and the use of it here (rather than in value numbering)
> is even more mysterious.

Sorry, that was left-over debugging code. I used it to find the reason for
infinite recursion, which was that replacing conj([]) with conj([]) does not
get strictly closer to the end of the documented well-founded order. I will
remove it.

> > +		( ReorderConj = yes
> > +		; FullyStrict = yes
> > +		)
> > +	->
> > +		MoveAny = no
> 
> Shouldn't you be checking `ReorderConj = no' there
> rather than `ReorderConj = yes'?

You are right.

> I don't think this is sufficient.
> Here you check that you don't move an impure goal across a pure goal.
> But doing the converse, i.e. moving a pure goal across an impure goal,
> is equally bad, and this test won't prevent that.

I have a proposed solution, which I will discuss with you in person.

> > +:- pred dead_code__cannot_diverge(hlds_goal::in) is semidet.
> 
> There's already a predicate code_aux__goal_cannot_loop
> which does the same thing as this one, so you should
> use that rather than duplicating the logic here.

Actually, code_aux__goal_cannot_loop was only documented to do the same
thing as dead_code__cannot_diverge, but its code did not check for
throwing exceptions. I have now added a new predicate, 
code_aux__goal_cannot_loop_or_throw, that does this check,
and deleted dead_code__cannot_diverge.

> goal_path.m:
> > +fill_expr_slots(switch(Var, B, Cases0, D), Path0, SlotInfo,
> > +		switch(Var, B, Cases, D)) :-
> > +	SlotInfo = slot_info(VarTypes, ModuleInfo),
> > +	module_info_types(ModuleInfo, TypeTable),
> > +	map__lookup(VarTypes, Var, Type),
> > +	(
> > +		type_to_type_id(Type, TypeId, _),
> > +		( map__search(TypeTable, TypeId, TypeDefn) ->
> > +			hlds_data__get_type_defn_body(TypeDefn, Body),
> > +			Body = du_type(_, ConsTable, _, _),
> > +			map__count(ConsTable, Count)
> > +		; TypeId = unqualified("character") - 0 ->
> > +			char__max_char_value(MaxCharValue),
> > +			char__min_char_value(MinCharValue),
> > +			Count = MaxCharValue - MinCharValue + 1
> > +		;
> > +			Count = -1
> > +		)
> 
> There should be an XXX comment here like the one in
> `switch_covers_all_cases' in switch_detection.m.  In fact a lot
> of the code here is doing the same thing as code in
> `switch_covers_all_cases'; it would be much better to factor out
> the common code into a separate subroutine, called e.g.
> `num_alternatives_in_type'.

I have factored out the common code into a new predicate
type_util__switch_type_num_functors.

> > +	proc_info_headvars(ProcInfo, HeadVars),
> > +	proc_info_argmodes(ProcInfo, ArgModes),
> > +	assoc_list__from_corresponding_lists(HeadVars, ArgModes, HeadVarModes),
> > +	IsInstChanged = lambda([VarMode::in, Var::out] is semidet, (
> > +		VarMode = Var - Mode,
> > +		mode_get_insts(ModuleInfo, Mode, Inst1, Inst2),
> > +		\+ inst_matches_binding(Inst1, Inst2, ModuleInfo)
> 
> ... since the implementation uses `inst_matches_binding',
> which ignores uniqueness, and which fails for `any' insts.

What is the correct test then?

Zoltan.
--------------------------------------------------------------------------
mercury-developers mailing list
Post messages to:       mercury-developers at cs.mu.oz.au
Administrative Queries: owner-mercury-developers at cs.mu.oz.au
Subscriptions:          mercury-developers-request at cs.mu.oz.au
--------------------------------------------------------------------------