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

[Zoltan Somogyi]

2021-10-13 17:05 GMT+11:00 "Peter Wang" <novalazy at gmail.com>:
>> +    % The second transformation is done by lco_transform_variant_plain_call.
>> +    % It checks whether the callee of a plain call goal has an lco-optimized
>> +    % variants that is applicable to the call site, and if yes, calls that
> 
> variant (no s)

Done.


>> +            % We want to process the conjuncts of the conjunction backwards,
>> +            % from last conjunct to first conjunct, so that when we process
>> +            % a conjunct, we know what variables the conjuncts to its right
>> +            % need as inputs. (This info is needed for the fix of Mantis
>> +            % bug #539.
> 
> Close bracket.


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

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


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

>> +            % The need for the test done by the rest of this condition
>> +            % is explained by the comment on the variant_transforms type.
> 
> Or:
> 
>     Do not transform a call if the transform would replace an
>     output variable needed by later code with a different variable.

I think that would be misleading. The output variable still sort-of
exists, it is just in a heap cell, and not in a form that is available
to later conjuncts.

>> +            Transforms = groundings_and_lco_calls(NeededVarsSet),
>> +            set_of_var.list_to_set(OutArgVars, OutArgVarsSet),
>> +            set_of_var.intersect(NeededVarsSet, OutArgVarsSet,
>> +                NeededOutArgVarsSet),
>> +            set_of_var.is_empty(NeededOutArgVarsSet)
> 
> You only need to intersect with VariantArgVars instead of OutArgVars,
> right?

Yep, I think you are right. I will test that change.

> The rest looks fine. Thanks for looking at it.

You are welcome.

Zoltan.