for review: --trace shallow variable liveness problem (2)

[Peter Wang-6]

Branches: main

Revert a change committed on 2009-08-21 to prevent a compiler abort when using
--trace shallow on a test case.  The change caused aborts with other modules.
Fix the original bug a different way, and another bug with --trace shallow.

compiler/live_vars.m:
        Revert the old change.

        Add a check that variables needed across a call do not appear in the
        post-death set of that call.

        Only add typeinfo variables to the set of variables needed across a
        call for output variable which are not local to the call.  Otherwise
        the sanity check fails on livevars_shallow2.m.

compiler/liveness.m:
        A switch variable which needs to be added to the pre-death set of a
        case may require typeinfo variables which describe the type of that
        variable to be added to the pre-death set as well.  This fixes the
        problem with livevars_shallow.m.

tests/valid/livevars_shallow.m:
        Replace this test case by another derived from the same original file.

tests/valid/Mercury.options:
tests/valid/Mmakefile:
tests/valid/livevars_shallow2.m:
        Add test case.

diff --git a/compiler/live_vars.m b/compiler/live_vars.m
index 6e5edeb..fb8e2cb 100644
--- a/compiler/live_vars.m
+++ b/compiler/live_vars.m
@@ -475,30 +475,36 @@ build_live_sets_in_call(OutVars, GoalInfo0, GoalInfo, ResumeVars0, AllocData,
     set.difference(Liveness, OutVars, ForwardVars0),
 
     % Might need to add more live variables with typeinfo liveness
-    % calculation.
+    % calculation. We only add the typeinfo variables for nonlocal outputs.
 
+    NonLocals = goal_info_get_nonlocals(GoalInfo0),
+    set.intersect(OutVars, NonLocals, NonLocalOutVars),
     maybe_add_typeinfo_liveness(AllocData ^ ad_proc_info,
-        AllocData ^ ad_typeinfo_liveness, OutVars, ForwardVars0, ForwardVars),
+        AllocData ^ ad_typeinfo_liveness, NonLocalOutVars,
+        ForwardVars0, ForwardVars),
+
+    % Sanity check.
+    goal_info_get_post_deaths(GoalInfo0, PostDeaths0),
+    set.intersect(PostDeaths0, ForwardVars, Intersect),
+    ( set.empty(Intersect) ->
+        true
+    ;
+        unexpected(this_file,
+            "build_live_sets_in_call: ForwardVars intersects post-death set")
+    ),
 
     Detism = goal_info_get_determinism(GoalInfo0),
     (
         Detism = detism_erroneous,
         AllocData ^ ad_opt_no_return_calls = yes
     ->
-        NeedAcrossCall = need_across_call(set.init, set.init, set.init),
-        GoalInfo1 = GoalInfo0
+        NeedAcrossCall = need_across_call(set.init, set.init, set.init)
     ;
         NeedAcrossCall = need_across_call(ForwardVars, ResumeVars0,
-            !.NondetLiveness),
-        % Make sure a variable doesn't appear in both the forward vars and
-        % post-death set.  (Perhaps the same thing needs to be done for
-        % other need_across_call sets?)
-        goal_info_get_post_deaths(GoalInfo0, PostDeaths0),
-        set.difference(PostDeaths0, ForwardVars, PostDeaths1),
-        goal_info_set_post_deaths(PostDeaths1, GoalInfo0, GoalInfo1)
+            !.NondetLiveness)
     ),
 
-    record_call_site(NeedAcrossCall, GoalInfo1, GoalInfo, !StackAlloc),
+    record_call_site(NeedAcrossCall, GoalInfo0, GoalInfo, !StackAlloc),
 
     % If this is a nondet call, then all the stack slots we need
     % must be protected against reuse in following code.
diff --git a/compiler/liveness.m b/compiler/liveness.m
index a62f656..37352c1 100644
--- a/compiler/liveness.m
+++ b/compiler/liveness.m
@@ -865,10 +865,12 @@ detect_deadness_in_disj([Goal0 | Goals0], [Goal | Goals], Deadness0, Liveness0,
     set(prog_var)::in, set(prog_var)::out, set(prog_var)::out) is det.
 
 detect_deadness_in_cases(SwitchVar, [], [], _Deadness0, _Liveness,
-        CompletedNonLocals, _LiveInfo, !Union, CompletedNonLocalUnion) :-
-    % If the switch variable does not become dead in a case,
-    % it must be put in the pre-death set of that case.
-    set.insert(!.Union, SwitchVar, !:Union),
+        CompletedNonLocals, LiveInfo, !Union, CompletedNonLocalUnion) :-
+    % If the switch variable does not become dead in a case, it must be put in
+    % the pre-death set of that case.
+    maybe_complete_with_typeinfos(LiveInfo, set.make_singleton_set(SwitchVar),
+        CompletedSwitchVar),
+    set.union(CompletedSwitchVar, !Union),
     set.intersect(!.Union, CompletedNonLocals, CompletedNonLocalUnion).
 detect_deadness_in_cases(SwitchVar, [Case0 | Cases0], [Case | Cases],
         Deadness0, Liveness0, CompletedNonLocals, LiveInfo, !Union,
diff --git a/tests/valid/Mercury.options b/tests/valid/Mercury.options
index ab37092..7e87400 100644
--- a/tests/valid/Mercury.options
+++ b/tests/valid/Mercury.options
@@ -97,6 +97,7 @@ MCFLAGS-intermod_user_sharing_2 = --intermodule-optimization
 MCFLAGS-lambda_inference = --infer-all
 MCFLAGS-livevals_seq = -O5 --opt-space
 MCFLAGS-livevars_shallow = --grade none --trace shallow
+MCFLAGS-livevars_shallow2 = --grade none --trace shallow
 MCFLAGS-lco_term = --optimize-constructor-last-call
 MCFLAGS-loop_inv_bug = --common-struct --loop-invariants
 MCFLAGS-mc_bag = --prop-mode-constraints
diff --git a/tests/valid/Mmakefile b/tests/valid/Mmakefile
index e0107ee..111d771 100644
--- a/tests/valid/Mmakefile
+++ b/tests/valid/Mmakefile
@@ -264,6 +264,7 @@ LLDS_PROGS= \
  exists_cast_bug \
  fzn_debug_abort \
  livevars_shallow \
+ livevars_shallow2 \
  untuple_bug
 
 # These tests only work in grades that support tabling.
diff --git a/tests/valid/livevars_shallow.m b/tests/valid/livevars_shallow.m
index e7763d4..c67d73f 100644
--- a/tests/valid/livevars_shallow.m
+++ b/tests/valid/livevars_shallow.m
@@ -10,45 +10,50 @@
 :- module livevars_shallow.
 :- interface.
 
-:- pred update({}::out) is det.
+:- import_module list.
 
-%-----------------------------------------------------------------------------%
-%-----------------------------------------------------------------------------%
-
-:- implementation.
-
-:- type net(AtomicTask)
-    --->    net.
+:- type node(A)
+    --->    place
+    ;       task
+    .
 
-:- type task(AtomicTask)
-    --->    task.
-
-:- type task_type(AtomicTask)
-    --->    atomik
+:- type task_type(A)
+    --->    atomick
     ;       composite.
 
+:- pred cancellation_set_updates(list(node(A))::in, list(string)::out) is det.
+
+%-----------------------------------------------------------------------------%
 %-----------------------------------------------------------------------------%
 
-update(AtomicTasks) :-
-    Net = net : net(int),
-    classify_nodes(Net, AtomicTasks).
+:- implementation.
+
+cancellation_set_updates(Nodes, L) :-
+    classify_nodes(Nodes, L).
 
-:- pred classify_nodes(net(AtomicTask)::in, {}::out) is det.
+:- pred classify_nodes(list(node(A))::in, list(string)::out) is det.
 
-classify_nodes(_ : net(AtomicTask), AtomicTasks) :-
-    Task = task : task(AtomicTask),
-    TaskType = get_task_type(Task),
+classify_nodes([], []).
+classify_nodes([Node | Nodes], L) :-
     (
-        TaskType = atomik,
-        AtomicTasks = {}
+        Node = place,
+        classify_nodes(Nodes, L)
     ;
-        TaskType = composite,
-        AtomicTasks = {}
+        Node = task,
+        classify_nodes(Nodes, L0),
+        TaskType = get_task_type(Node),
+        (
+            TaskType = atomick,
+            L = L0
+        ;
+            TaskType = composite,
+            L = L0
+        )
     ).
 
-:- func get_task_type(task(AtomicTask)) = task_type(AtomicTask).
+:- func get_task_type(node(A)) = task_type(A).
 
-get_task_type(_) = atomik.
+get_task_type(_) = composite.
 
 %-----------------------------------------------------------------------------%
 % vim: set sts=4 sw=4 et:
diff --git a/tests/valid/livevars_shallow2.m b/tests/valid/livevars_shallow2.m
new file mode 100644
index 0000000..00793f4
--- /dev/null
+++ b/tests/valid/livevars_shallow2.m
@@ -0,0 +1,50 @@
+%-----------------------------------------------------------------------------%
+% Regression test.
+% The compiler aborted on this module with --trace shallow due to an incorrect
+% fix.
+%-----------------------------------------------------------------------------%
+
+:- module livevars_shallow2.
+:- interface.
+
+:- import_module io.
+
+:- pred main(io::di, io::uo) is det.
+
+%-----------------------------------------------------------------------------%
+%-----------------------------------------------------------------------------%
+
+:- implementation.
+
+:- type ps ---> ps(int).
+
+%-----------------------------------------------------------------------------%
+
+main(!IO) :-
+    P = stringify_state(int_with_state),
+    io.write(P, !IO),
+    io.nl(!IO).
+
+:- pred int_with_state(int::out, ps::in, ps::out) is semidet.
+
+int_with_state(1, !PS) :-
+    semidet_true.
+
+:- pred stringify_state(
+        pred(T, ps, ps)::
+            in(pred(out, in, out) is semidet),
+        string::out,
+        ps::in,
+        ps::out)
+        is semidet.
+
+stringify_state(P, String, !PS) :-
+    P(_, !PS),
+    String = xfoo(-1).
+
+:- func xfoo(T) = string.
+
+xfoo(_) = "xfoo".
+
+%-----------------------------------------------------------------------------%
+% vim: set sts=4 sw=4 et:


--------------------------------------------------------------------------
mercury-reviews mailing list
Post messages to:       mercury-reviews@...
Administrative Queries: owner-mercury-reviews@...
Subscriptions:          mercury-reviews-request@...
--------------------------------------------------------------------------

[Peter Ross-6]

On Wed, Oct 28, 2009 at 3:45 PM, Peter Wang <novalazy@...> wrote:

> Branches: main
>
> Revert a change committed on 2009-08-21 to prevent a compiler abort when using
> --trace shallow on a test case.  The change caused aborts with other modules.
> Fix the original bug a different way, and another bug with --trace shallow.
>
> compiler/live_vars.m:
>        Revert the old change.
>
>        Add a check that variables needed across a call do not appear in the
>        post-death set of that call.
>
>        Only add typeinfo variables to the set of variables needed across a
>        call for output variable which are not local to the call.  Otherwise
>        the sanity check fails on livevars_shallow2.m.
>
> compiler/liveness.m:
>        A switch variable which needs to be added to the pre-death set of a
>        case may require typeinfo variables which describe the type of that
>        variable to be added to the pre-death set as well.  This fixes the
>        problem with livevars_shallow.m.
>
> tests/valid/livevars_shallow.m:
>        Replace this test case by another derived from the same original file.
>
> tests/valid/Mercury.options:
> tests/valid/Mmakefile:
> tests/valid/livevars_shallow2.m:
>        Add test case.
>

I'm not an expert in this area, but for me the diff looks fine.

--------------------------------------------------------------------------
mercury-reviews mailing list
Post messages to:       mercury-reviews@...
Administrative Queries: owner-mercury-reviews@...
Subscriptions:          mercury-reviews-request@...
--------------------------------------------------------------------------

[Julien Fischer-2]

On Wed, 28 Oct 2009, Peter Wang wrote:

> diff --git a/compiler/live_vars.m b/compiler/live_vars.m
> index 6e5edeb..fb8e2cb 100644
> --- a/compiler/live_vars.m
> +++ b/compiler/live_vars.m
> @@ -475,30 +475,36 @@ build_live_sets_in_call(OutVars, GoalInfo0, GoalInfo, ResumeVars0, AllocData,
>     set.difference(Liveness, OutVars, ForwardVars0),
>
>     % Might need to add more live variables with typeinfo liveness
> -    % calculation.
> +    % calculation. We only add the typeinfo variables for nonlocal outputs.
>
> +    NonLocals = goal_info_get_nonlocals(GoalInfo0),
> +    set.intersect(OutVars, NonLocals, NonLocalOutVars),
>     maybe_add_typeinfo_liveness(AllocData ^ ad_proc_info,
> -        AllocData ^ ad_typeinfo_liveness, OutVars, ForwardVars0, ForwardVars),
> +        AllocData ^ ad_typeinfo_liveness, NonLocalOutVars,
> +        ForwardVars0, ForwardVars),
> +
> +    % Sanity check.
> +    goal_info_get_post_deaths(GoalInfo0, PostDeaths0),
> +    set.intersect(PostDeaths0, ForwardVars, Intersect),
> +    ( set.empty(Intersect) ->
> +        true
> +    ;
> +        unexpected(this_file,
> +            "build_live_sets_in_call: ForwardVars intersects post-death set")
> +    ),

This sanity check is being tripped when compiling the tree234 module in
asm_fast.gc.decldebug (with --intermodule-optimisation -O5 --opt-space,
but most of that is probably incidental.)

If this is not something that is easily fixed, could you please back
this out -- it's breaking the nightly builds on a lot of our mcahines.

Julien.

...

% Generating code for predicate
% `tree234.IntroducedFrom__pred__do_from_sorted_assoc_list__3199__1'/2
% Generating code for predicate
% `tree234.IntroducedFrom__pred__do_from_sorted_assoc_list__3246__1'/2
% Generating code for predicate
% `tree234.IntroducedFrom__pred__do_from_rev_sorted_assoc_list__3307__1'/2
% Generating code for predicate
% `tree234.IntroducedFrom__pred__do_from_rev_sorted_assoc_list__3340__1'/2
% Generating code for predicate
% `tree234.IntroducedFrom__pred__do_from_rev_sorted_assoc_list__3387__1'/2
% Generating code for predicate `tree234.TypeSpecOf__pred__set3__[K =
% var(V_2)]_1_1'/4
% Generating code for predicate `tree234.TypeSpecOf__pred__set2__[K =
% var(V_2)]_1_1'/4
% Generating code for predicate `tree234.TypeSpecOf__pred__update__[K =
% int]_0_1'/4
% Generating code for predicate `tree234.TypeSpecOf__pred__update__[K =
% var(V_2)]_0_1'/4
% Generating code for predicate `tree234.TypeSpecOf__pred__set__[K =
% var(V_2)]_0_1'/4
% Generating code for predicate
% `tree234.TypeSpecOf__pred_or_func__lookup__[K = var(V_2)]_0_1'/3
Uncaught Mercury exception:
Software Error: live_vars.m: Unexpected: build_live_sets_in_call:
ForwardVars intersects post-death set
Stack dump not available in this grade.
--------------------------------------------------------------------------
mercury-reviews mailing list
Post messages to:       mercury-reviews@...
Administrative Queries: owner-mercury-reviews@...
Subscriptions:          mercury-reviews-request@...
--------------------------------------------------------------------------

[Peter Wang-6]

On 2009-11-01, Julien Fischer <juliensf@...> wrote:

> >+    % Sanity check.
> >+    goal_info_get_post_deaths(GoalInfo0, PostDeaths0),
> >+    set.intersect(PostDeaths0, ForwardVars, Intersect),
> >+    ( set.empty(Intersect) ->
> >+        true
> >+    ;
> >+        unexpected(this_file,
> >+            "build_live_sets_in_call: ForwardVars intersects post-death set")
> >+    ),
>
> This sanity check is being tripped when compiling the tree234 module in
> asm_fast.gc.decldebug (with --intermodule-optimisation -O5 --opt-space,
> but most of that is probably incidental.)
>
> If this is not something that is easily fixed, could you please back
> this out -- it's breaking the nightly builds on a lot of our mcahines.

I've backed out the changes to this file.

Branches: main

compiler/live_vars.m:
        Revert check that variables needed across a call do not appear in the
        post-death set of that call.  This was breaking at least
        asm_fast.gc.decldebug.

        Revert a related change that was needed to pass the livevars_shallow2.m
        test case when the previous sanity check was enabled.

diff --git a/compiler/live_vars.m b/compiler/live_vars.m
index fb8e2cb..8dd3c4a 100644
--- a/compiler/live_vars.m
+++ b/compiler/live_vars.m
@@ -475,23 +475,10 @@ build_live_sets_in_call(OutVars, GoalInfo0, GoalInfo, ResumeVars0, AllocData,
     set.difference(Liveness, OutVars, ForwardVars0),
 
     % Might need to add more live variables with typeinfo liveness
-    % calculation. We only add the typeinfo variables for nonlocal outputs.
+    % calculation.
 
-    NonLocals = goal_info_get_nonlocals(GoalInfo0),
-    set.intersect(OutVars, NonLocals, NonLocalOutVars),
     maybe_add_typeinfo_liveness(AllocData ^ ad_proc_info,
-        AllocData ^ ad_typeinfo_liveness, NonLocalOutVars,
-        ForwardVars0, ForwardVars),
-
-    % Sanity check.
-    goal_info_get_post_deaths(GoalInfo0, PostDeaths0),
-    set.intersect(PostDeaths0, ForwardVars, Intersect),
-    ( set.empty(Intersect) ->
-        true
-    ;
-        unexpected(this_file,
-            "build_live_sets_in_call: ForwardVars intersects post-death set")
-    ),
+        AllocData ^ ad_typeinfo_liveness, OutVars, ForwardVars0, ForwardVars),
 
     Detism = goal_info_get_determinism(GoalInfo0),
     (
--------------------------------------------------------------------------
mercury-reviews mailing list
Post messages to:       mercury-reviews@...
Administrative Queries: owner-mercury-reviews@...
Subscriptions:          mercury-reviews-request@...
--------------------------------------------------------------------------