beam_jump: Fail label of select_val is unsafe for move elimination

Consider the following code:

bme(Int) ->
    TagInt = Int band 2#111,
    Tag = case TagInt of
              0 -> a; 1 -> b; 2 -> c; 3 -> d;
              4 -> e; 5 -> f; 6 -> g; 7 -> h
          end,
    case Tag of
        g -> expects_g(TagInt, Tag);
        h -> expects_h(TagInt, Tag);
        _ -> Tag = id(Tag), ok
    end.

expects_g(6, Atom) -> Atom = id(g), ok.
expects_h(7, Atom) -> Atom = id(h), ok.

The type optimization pass would recognize that TagInt can only be
[0 .. 7], so the first 'case' would select_val over [0 .. 6] and swap
out the fail label with the block for 7.

A later optimization would merge this block with 'expects_h' in the
second case, as the latter is only reachable from the former.

... but this broke down when the move elimination optimization didn't
take the fail label of the first select_val into account. This caused it
believe that the only way to reach 'expects_h' was through the second
case when 'Tag' =:= 'h', which made it remove the move instruction
added in the first case, passing garbage to expects_h/2.

2 files changed, 49 insertions, 6 deletions

diff --git a/lib/compiler/src/beam_jump.erl b/lib/compiler/src/beam_jump.erl
index 6f50bfdb9c..74f80ca70e 100644
--- a/lib/compiler/src/beam_jump.erl
+++ b/lib/compiler/src/beam_jump.erl
@@ -179,8 +179,9 @@ function({function,Name,Arity,CLabel,Asm0}, Lc0) ->
 eliminate_moves(Is) ->
     eliminate_moves(Is, #{}, []).
 
-eliminate_moves([{select,select_val,Reg,_,List}=I|Is], D0, Acc) ->
-    D = update_value_dict(List, Reg, D0),
+eliminate_moves([{select,select_val,Reg,{f,Fail},List}=I|Is], D0, Acc) ->
+    D1 = add_unsafe_label(Fail, D0),
+    D = update_value_dict(List, Reg, D1),
     eliminate_moves(Is, D, [I|Acc]);
 eliminate_moves([{test,is_eq_exact,_,[Reg,Val]}=I,
                  {block,BlkIs0}|Is], D0, Acc) ->
@@ -229,6 +230,9 @@ update_value_dict([Lit,{f,Lbl}|T], Reg, D0) ->
     update_value_dict(T, Reg, D);
 update_value_dict([], _, D) -> D.
 
+add_unsafe_label(L, D) ->
+    D#{L=>unsafe}.
+
 update_unsafe_labels(I, D) ->
     Ls = instr_labels(I),
     update_unsafe_labels_1(Ls, D).
diff --git a/lib/compiler/test/beam_jump_SUITE.erl b/lib/compiler/test/beam_jump_SUITE.erl
index 759d884dc4..a456f31d79 100644
--- a/lib/compiler/test/beam_jump_SUITE.erl
+++ b/lib/compiler/test/beam_jump_SUITE.erl
@@ -79,12 +79,13 @@ checks(Wanted) ->
     {catch case river() of sheet -> begin +Wanted, if "da" -> Wanted end end end, catch case river() of sheet -> begin + Wanted, if "da" -> Wanted end end end}.
 
 unsafe_move_elimination(_Config) ->
-    {{left,right,false},false} = unsafe_move_elimination(left, right, false),
-    {{false,right,false},false} = unsafe_move_elimination(false, right, true),
-    {{true,right,right},right} = unsafe_move_elimination(true, right, true),
+    {{left,right,false},false} = unsafe_move_elimination_1(left, right, false),
+    {{false,right,false},false} = unsafe_move_elimination_1(false, right, true),
+    {{true,right,right},right} = unsafe_move_elimination_1(true, right, true),
+    [ok = unsafe_move_elimination_2(I) || I <- lists:seq(0,16)],
     ok.
 
-unsafe_move_elimination(Left, Right, Simple0) ->
+unsafe_move_elimination_1(Left, Right, Simple0) ->
     id(1),
 
     %% The move at label 29 would be removed by beam_jump, which is unsafe because
@@ -115,6 +116,44 @@ unsafe_move_elimination(Left, Right, Simple0) ->
              end,
     {id({Left,Right,Simple}),Simple}.
 
+unsafe_move_elimination_2(Int) ->
+    %% The type optimization pass would recognize that TagInt can only be
+    %% [0 .. 7], so the first 'case' would select_val over [0 .. 6] and swap
+    %% out the fail label with the block for 7.
+    %%
+    %% A later optimization would merge this block with 'expects_h' in the
+    %% second case, as the latter is only reachable from the former.
+    %%
+    %% ... but this broke down when the move elimination optimization didn't
+    %% take the fail label of the first select_val into account. This caused it
+    %% to believe that the only way to reach 'expects_h' was through the second
+    %% case when 'Tag' =:= 'h', which made it remove the move instruction
+    %% added in the first case, passing garbage to expects_h/2.
+    TagInt = Int band 2#111,
+    Tag = case TagInt of
+              0 -> a;
+              1 -> b;
+              2 -> c;
+              3 -> d;
+              4 -> e;
+              5 -> f;
+              6 -> g;
+              7 -> h
+          end,
+    case Tag of
+        g -> expects_g(TagInt, Tag);
+        h -> expects_h(TagInt, Tag);
+        _ -> Tag = id(Tag), ok
+    end.
+
+expects_g(6, Atom) ->
+    Atom = id(g),
+    ok.
+
+expects_h(7, Atom) ->
+    Atom = id(h),
+    ok.
+
 -record(message2, {id, p1}).
 -record(message3, {id, p1, p2}).