[m-rev.] for review: Fix computation of bound variables in goal_info_to_atomic_goal_rep_fields.

[Peter Wang]

On Mon, 18 Aug 2014 16:06:37 +1000, Peter Wang <novalazy at gmail.com> wrote:
> compiler/prog_rep.m:
> 	Use `instmap_changed_vars_old' in a spot to avoid breaking some
> 	declarative debugging test cases.  I don't know where the real
> 	problem lies.

---

Fix computation of bound variables in goal_info_to_atomic_goal_rep_fields.

instmap_changed_vars calls inst_matches_final_typed to compare the insts
of a variable between two instmaps.  But inst_matches_final_typed
contained a hack such that ground matches bound(...), so variables
changing instantiatedness from ground to bound(...) would NOT be
included in the result set of instmap_changed_vars.  Once the hack was
removed, the behaviour of instmap_changed_vars changed to match its
documentation.

goal_info_to_atomic_goal_rep_fields used instmap_changed_vars to compute
BoundVars.  Variables whose instantiation state changes from ground to
bound, but are not actually bound by the goal, would now be in BoundVars.
This bug led to changes in the goal representation (e.g. from
unify_deconstruct_rep to partial_deconstruct_rep) and caused some
declarative debugging test cases to fail.

compiler/prog_rep.m:
	Change how goal_info_to_atomic_goal_rep_fields to exclude
	variables which are ground before the goal from BoundVars.

	Reorder some code to execution order.

compiler/instmap.m:
	Delete instmap_changed_vars_old.
---
 compiler/instmap.m  | 29 ++++++-----------------------
 compiler/prog_rep.m | 15 ++++++++-------
 2 files changed, 14 insertions(+), 30 deletions(-)

diff --git a/compiler/instmap.m b/compiler/instmap.m
index d895a27..907b52f 100644
--- a/compiler/instmap.m
+++ b/compiler/instmap.m
@@ -121,13 +121,6 @@
 :- pred instmap_changed_vars(instmap::in, instmap::in, vartypes::in,
     module_info::in, set_of_progvar::out) is det.
 
-    % The old behaviour of instmap_changed_vars.
-    % This should only be used by goal_info_to_atomic_goal_rep_fields
-    % until the real problem is found.
-    %
-:- pred instmap_changed_vars_old(instmap::in, instmap::in, vartypes::in,
-    module_info::in, set_of_progvar::out) is det.
-
 %-----------------------------------------------------------------------------%
 
     % Given an instmap and a variable, determine the inst of that variable.
@@ -508,34 +501,24 @@ instmap_delta_changed_vars(reachable(InstMapping), ChangedVars) :-
 instmap_changed_vars(InstMapA, InstMapB, VarTypes, ModuleInfo, ChangedVars) :-
     instmap_vars_list(InstMapB, VarsB),
     instmap_changed_vars_2(VarsB, InstMapA, InstMapB, VarTypes, ModuleInfo,
-        do_not_trust_final_insts, ChangedVars).
-
-instmap_changed_vars_old(InstMapA, InstMapB, VarTypes, ModuleInfo, ChangedVars)
-        :-
-    instmap_vars_list(InstMapB, VarsB),
-    instmap_changed_vars_2(VarsB, InstMapA, InstMapB, VarTypes, ModuleInfo,
-        trust_final_insts, ChangedVars).
+        ChangedVars).
 
 :- pred instmap_changed_vars_2(prog_vars::in, instmap::in, instmap::in,
-    vartypes::in, module_info::in, trust_final_insts::in, set_of_progvar::out)
-    is det.
+    vartypes::in, module_info::in, set_of_progvar::out) is det.
 
 instmap_changed_vars_2([], _InstMapA, _InstMapB, _Types,
-        _ModuleInfo, _TrustFinalInsts, ChangedVars) :-
+        _ModuleInfo, ChangedVars) :-
     set_of_var.init(ChangedVars).
 instmap_changed_vars_2([VarB | VarBs], InstMapA, InstMapB, VarTypes,
-        ModuleInfo, TrustFinalInsts, ChangedVars) :-
+        ModuleInfo, ChangedVars) :-
     instmap_changed_vars_2(VarBs, InstMapA, InstMapB, VarTypes,
-        ModuleInfo, TrustFinalInsts, ChangedVars0),
+        ModuleInfo, ChangedVars0),
 
     instmap_lookup_var(InstMapA, VarB, InitialInst),
     instmap_lookup_var(InstMapB, VarB, FinalInst),
     lookup_var_type(VarTypes, VarB, Type),
 
-    (
-        inst_matches_final_typed(InitialInst, FinalInst, Type, ModuleInfo,
-            TrustFinalInsts)
-    ->
+    ( inst_matches_final_typed(InitialInst, FinalInst, Type, ModuleInfo) ->
         ChangedVars = ChangedVars0
     ;
         set_of_var.insert(VarB, ChangedVars0, ChangedVars)
diff --git a/compiler/prog_rep.m b/compiler/prog_rep.m
index 3dd4914..b8c366c 100644
--- a/compiler/prog_rep.m
+++ b/compiler/prog_rep.m
@@ -498,6 +498,9 @@ goal_to_goal_rep(Info, Instmap0, hlds_goal(GoalExpr, GoalInfo), GoalRep) :-
         ; GoalExpr = plain_call(_, _, _, _, _, _)
         ; GoalExpr = call_foreign_proc(_, _, _, _, _, _, _)
         ),
+        goal_info_to_atomic_goal_rep_fields(GoalInfo, Instmap0, Info,
+            FileName, LineNo, BoundVars),
+        BoundVarsRep = map(var_to_var_rep(Info), BoundVars),
         (
             GoalExpr = unify(_, _, _, Uni, _),
             (
@@ -587,9 +590,6 @@ goal_to_goal_rep(Info, Instmap0, hlds_goal(GoalExpr, GoalInfo), GoalRep) :-
                 compose(var_to_var_rep(Info), foreign_arg_var), Args),
             AtomicGoalRep = pragma_foreign_code_rep(ArgVarsRep)
         ),
-        goal_info_to_atomic_goal_rep_fields(GoalInfo, Instmap0, Info,
-            FileName, LineNo, BoundVars),
-        BoundVarsRep = map(var_to_var_rep(Info), BoundVars),
         GoalExprRep = atomic_goal_rep(FileName, LineNo, BoundVarsRep,
             AtomicGoalRep)
     ;
@@ -840,10 +840,11 @@ goal_info_to_atomic_goal_rep_fields(GoalInfo, Instmap0, Info, FileName, LineNo,
     ),
     term.context_line(Context, LineNo),
     InstmapDelta = goal_info_get_instmap_delta(GoalInfo),
-    instmap.apply_instmap_delta(Instmap0, InstmapDelta, Instmap),
-    instmap_changed_vars_old(Instmap0, Instmap, Info ^ pri_vartypes,
-        Info ^ pri_module_info, ChangedVars),
-    set_of_var.to_sorted_list(ChangedVars, BoundVars).
+    instmap_delta_changed_vars(InstmapDelta, ChangedVarsSet),
+    set_of_var.to_sorted_list(ChangedVarsSet, ChangedVars),
+    ModuleInfo = Info ^ pri_module_info,
+    list.negated_filter(var_is_ground_in_instmap(ModuleInfo, Instmap0),
+        ChangedVars, BoundVars).
 
 :- pred encode_cons_id_and_arity_rep(cons_id_arity_rep::in, list(int)::out,
     string_table_info::in, string_table_info::out) is det.
-- 
1.8.4