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

[Peter Wang]

On Wed, 13 Oct 2021 17:19:03 +1100 "Zoltan Somogyi" <zoltan.somogyi at runbox.com> wrote:
> >>          GoalExpr0 = if_then_else(Vars, Cond, Then0, Else0),
> >>          update_instmap(Cond, InstMap0, InstMap1),
> >> -        lco_transform_variant_goal(ModuleInfo, VariantMap, VarToAddr, InstMap1,
> >> -            Then0, Then, ThenChanged, !ProcInfo),
> >> -        lco_transform_variant_goal(ModuleInfo, VariantMap, VarToAddr, InstMap0,
> >> -            Else0, Else, ElseChanged, !ProcInfo),
> >> +        lco_transform_variant_goal(ModuleInfo, Transforms, VariantMap,
> >> +            VarToAddr, InstMap1, Then0, Then, ThenChanged, !ProcInfo),
> >> +        lco_transform_variant_goal(ModuleInfo, Transforms, VariantMap,
> >> +            VarToAddr, InstMap0, Else0, Else, ElseChanged, !ProcInfo),
> >>          Changed = bool.or(ThenChanged, ElseChanged),
> >>          GoalExpr = if_then_else(Vars, Cond, Then, Else),
> >>          GoalInfo = GoalInfo0
> > 
> > Reorder the two calls?
> 
> Why? We normally process then goals before else goals.

Nevermind, I was still thinking about processing goals in reverse
but that doesn't apply here.

> 
> >> +:- pred lco_transform_variant_rev_conj(module_info::in, variant_transforms::in,
> >> +    variant_map::in, var_to_target::in,
> >> +    list(goal_and_init_instmap)::in, cord(hlds_goal)::in, cord(hlds_goal)::out,
> >>      bool::out, proc_info::in, proc_info::out) is det.
> >>  
> >> -lco_transform_variant_conj(_, _, _, _, [], [], no, !ProcInfo).
> >> -lco_transform_variant_conj(ModuleInfo, VariantMap, VarToAddr, InstMap0,
> >> -        [Goal0 | Goals0], Conj, Changed, !ProcInfo) :-
> >> -    lco_transform_variant_goal(ModuleInfo, VariantMap, VarToAddr, InstMap0,
> >> -        Goal0, Goal, HeadChanged, !ProcInfo),
> >> -    update_instmap(Goal0, InstMap0, InstMap1),
> >> -    lco_transform_variant_conj(ModuleInfo, VariantMap, VarToAddr, InstMap1,
> >> -        Goals0, Goals, TailChanged, !ProcInfo),
> >> +lco_transform_variant_rev_conj(_, _, _, _, [], !Conjuncts, no, !ProcInfo).
> >> +lco_transform_variant_rev_conj(ModuleInfo, Transforms0, VariantMap, VarToAddr,
> >> +        [RevGoalIM0 | RevGoalIMs0], !Conjuncts, Changed, !ProcInfo) :-
> >> +    RevGoalIM0 = goal_and_init_instmap(RevGoal0, RevGoalInitInstMap),
> > 
> > Minor quibble: the "Rev" prefix on RevGoalIM0/RevGoal0/RevGoalInitInstMap
> > doesn't really make sense as it's not somehow "reversed".
> 
> The predicate is processing a reversed list, so yes, I would say they *are* reversed.

Ok.

> >> +:- pred get_input_vars_needed_by_goal(instmap::in, hlds_goal_info::in,
> >> +    set_of_progvar::out) is det.
> >> +get_input_vars_needed_by_goal(InstMap0, GoalInfo, NeededVars) :-
> >> +    % A goal may need the value of a variable from preceding code if
> >> +    % - the variable already has an inst before the goal, and
> >> +    % - the goal actually refers to the variable.
> >> +    instmap_vars(InstMap0, InstMapVars),
> >> +    NonLocals = goal_info_get_nonlocals(GoalInfo),
> >> +    NeededVars = set_of_var.intersect(InstMapVars, NonLocals).
> > 
> > Do you need to restrict InstMapVars to non-free variables?
> 
> We put variables into instmaps only on their first occurrence,
> and that first occurrence makes them more instantiated than "free"
> in the vast, vast majority of cases.
> 
> You are right, the code here is too conservative if a variable
> is in the instmap with inst "free", but the probability of that is
> so small that I don't think it is worth fixing.

Ok. It's worth a comment.

Peter