[m-rev.] for review: fix mantis bug 570

[Peter Wang]

On Wed, 13 Dec 2023 23:43:24 +1100 "Zoltan Somogyi" <zoltan.somogyi at runbox.com> wrote:
> Fix a bug in merging successive switches.

> diff --git a/compiler/simplify_goal_conj.m b/compiler/simplify_goal_conj.m
> index 725f0680e..747a083dc 100644
> --- a/compiler/simplify_goal_conj.m
> +++ b/compiler/simplify_goal_conj.m
...
> @@ -660,13 +706,67 @@ build_maps_second_switch([Case | Cases], CurCaseNum, FirstSwitchConsIdMap,
>  
>  build_maps_second_switch_cons_id(FirstSwitchConsIdMap, SecondSwitchCaseNum,
>          ConsId, !CasesConsIdsMap) :-
> -    map.lookup(FirstSwitchConsIdMap, ConsId, FirstSwitchCaseNum),
> -    CaseNums = {FirstSwitchCaseNum, SecondSwitchCaseNum},
> -    ( if map.search(!.CasesConsIdsMap, CaseNums, OldConsIds) then
> -        NewConsIds = one_or_more.cons(ConsId, OldConsIds),
> -        map.det_update(CaseNums, NewConsIds, !CasesConsIdsMap)
> +    ( if map.search(FirstSwitchConsIdMap, ConsId, FirstSwitchCaseNum) then
> +        CaseNums = {FirstSwitchCaseNum, SecondSwitchCaseNum},
> +        ( if map.search(!.CasesConsIdsMap, CaseNums, OldConsIds) then
> +            NewConsIds = one_or_more.cons(ConsId, OldConsIds),
> +            map.det_update(CaseNums, NewConsIds, !CasesConsIdsMap)
> +        else
> +            map.det_insert(CaseNums, one_or_more(ConsId, []), !CasesConsIdsMap)
> +        )
>      else
> -        map.det_insert(CaseNums, one_or_more(ConsId, []), !CasesConsIdsMap)
> +        % This cons_id exists in the first switch, but not in the second.

It should be the other way around.

> +        % This means one of two things.
> +        %
> +        % The first possibility is that the first switch is a can_fail switch.
> +        % In such a situation, if the value of the switched-on variable
> +        % is ConsId, then execution cannot reach the end of the first switch.
> +        % Mode analysis will notice this fact before the compiler even
> +        % discovers that the first switch *is* a switch, when it is still
> +        % a disjunction. It will generate a warning about the unification
> +        % of the same switched-on variable with ConsId in the disjunction
> +        % that would turn into the second switch, and then delete that
> +        % unification. So when switch detection gets around to that
> +        % disjunction, it won't have a unification with ConsId to make
> +        % ConsId one of the cons_ids of a switch arm. This means if execution
> +        % gets here, then this possibility cannot actually have happened.
> +        %
> +        % For an example of this, have a look at the HLDS dumps of
> +        % tests/hard_coded/bug570_can_fail.m from just before and just after
> +        % the mode checking pass.
> +        %
> +        % The second possibility is that the first switch is a cannot_fail
> +        % switch, but the inst of the switched-on variable at its start
> +        % implies that the switched-on variable *cannot* be bound to ConsId
> +        % there. Again, if the user writes code like this, the compiler will
> +        % notice that the unification of the switched-on variable with ConsId
> +        % in the disjunction that would turn into the second switch cannot
> +        % succeed, generate a warning, and then delete that unification.
> +        % This again would mean that execution cannot get here.
> +        %
> +        % However, there is a way in which execution *can* get here:
> +        % if the code sequence containing the two switches, the first
> +        % not having a ConsId arm but the second having such an arm,
> +        % is constructed internally by the compiler *after* the mode analysis
> +        % pass. This happens in tests/hard_coded/bug570, where this
> +        % construction is done by the deforestation pass, which then
> +        % invokes simplification on its output.
> +        %
> +        % The map.search above used to a map.lookup, but that test case showed

used to be

> +        % that it must be a map.search. The make_headers predicate in
> +        % bug570.m consists of three successive switches on the same variable,
> +        % Prepare, all of which handle all the function symbols of Prepare's
> +        % type. Deforestation moves copies of those switches into each arm
> +        % of the previous switch. In their new positions in the arms of the
> +        % first switch, some arms of the copied second switch become
> +        % unreachable, and are pruned away. Mantis bug #570 happened when
> +        % deforestation tried to move copies of the third switch, which had
> +        % an arm for Prepare = prepare_edit, into each arm of the merged
> +        % first/second switch, one of which occurred in a context in which
> +        % Prepare could *not* be bound to prepare_edit, because it was derived
> +        % from the arm of the first switch on Prepare which was *not*
> +        % prepare_edit.
> +        true
>      ).

That's fine otherwise, thanks.

Peter