Remove unsafe optimization for delaying creation of stackframe

b89044a800c4 introduced an optimization that tries to delay creation
of stack frames. It turns out that this optimization is not always
safe. (See the new test case for an example.)

Since the code generator is completely rewritten in the `master`
branch for the upcoming OTP 22 release, it does not make sense trying
to mend this optimization. It is better to remove it.  Out of a sample
of about 1000 modules in OTP, about 50 of them are changed as a result
of removing this optimization.

The compiler in OTP 22 will do the same optimization in a cleaner,
safer, and more effective way.

https://bugs.erlang.org/browse/ERL-807

2 files changed, 23 insertions, 135 deletions

diff --git a/lib/compiler/src/v3_codegen.erl b/lib/compiler/src/v3_codegen.erl
index e9152ba88f..d7a7778740 100644
--- a/lib/compiler/src/v3_codegen.erl
+++ b/lib/compiler/src/v3_codegen.erl
@@ -79,13 +79,9 @@ function(#k_fdef{anno=#k{a=Anno},func=Name,arity=Arity,
     try
         #k_match{} = Kb,                   %Assertion.
 
-        %% Try to suppress the stack frame unless it is
-        %% really needed.
-        Body0 = avoid_stack_frame(Kb),
-
         %% Annotate kernel records with variable usage.
         Vdb0 = init_vars(As),
-        {Body,_,Vdb} = body(Body0, 1, Vdb0),
+        {Body,_,Vdb} = body(Kb, 1, Vdb0),
 
         %% Generate the BEAM assembly code.
         {Asm,EntryLabel,St} = cg_fun(Body, As, Vdb, AtomMod,
@@ -98,136 +94,6 @@ function(#k_fdef{anno=#k{a=Anno},func=Name,arity=Arity,
             erlang:raise(Class, Error, Stack)
     end.
 
-
-%% avoid_stack_frame(Kernel) -> Kernel'
-%%  If possible, avoid setting up a stack frame. Functions
-%%  that only do matching, calls to guard BIFs, and tail-recursive
-%%  calls don't need a stack frame.
-
-avoid_stack_frame(#k_match{body=Body}=M) ->
-    try
-        M#k_match{body=avoid_stack_frame_1(Body)}
-    catch
-        impossible ->
-            M
-    end.
-
-avoid_stack_frame_1(#k_alt{first=First0,then=Then0}=Alt) ->
-    First = avoid_stack_frame_1(First0),
-    Then = avoid_stack_frame_1(Then0),
-    Alt#k_alt{first=First,then=Then};
-avoid_stack_frame_1(#k_bif{op=Op}=Bif) ->
-    case Op of
-        #k_internal{} ->
-            %% Most internal BIFs clobber the X registers.
-            throw(impossible);
-        _ ->
-            Bif
-    end;
-avoid_stack_frame_1(#k_break{anno=Anno,args=Args}) ->
-    #k_guard_break{anno=Anno,args=Args};
-avoid_stack_frame_1(#k_guard_break{}=Break) ->
-    Break;
-avoid_stack_frame_1(#k_enter{}=Enter) ->
-    %% Tail-recursive calls don't need a stack frame.
-    Enter;
-avoid_stack_frame_1(#k_guard{clauses=Cs0}=Guard) ->
-    Cs = avoid_stack_frame_list(Cs0),
-    Guard#k_guard{clauses=Cs};
-avoid_stack_frame_1(#k_guard_clause{guard=G0,body=B0}=C) ->
-    G = avoid_stack_frame_1(G0),
-    B = avoid_stack_frame_1(B0),
-    C#k_guard_clause{guard=G,body=B};
-avoid_stack_frame_1(#k_match{anno=A,vars=Vs,body=B0,ret=Ret}) ->
-    %% Use #k_guard_match{} instead to avoid saving the X registers
-    %% to the stack before matching.
-    B = avoid_stack_frame_1(B0),
-    #k_guard_match{anno=A,vars=Vs,body=B,ret=Ret};
-avoid_stack_frame_1(#k_guard_match{body=B0}=M) ->
-    B = avoid_stack_frame_1(B0),
-    M#k_guard_match{body=B};
-avoid_stack_frame_1(#k_protected{arg=Arg0}=Prot) ->
-    Arg = avoid_stack_frame_1(Arg0),
-    Prot#k_protected{arg=Arg};
-avoid_stack_frame_1(#k_put{}=Put) ->
-    Put;
-avoid_stack_frame_1(#k_return{}=Ret) ->
-    Ret;
-avoid_stack_frame_1(#k_select{var=#k_var{anno=Vanno},types=Types0}=Select) ->
-    case member(reuse_for_context, Vanno) of
-        false ->
-            Types = avoid_stack_frame_list(Types0),
-            Select#k_select{types=Types};
-        true ->
-            %% Including binary patterns that overwrite the register containing
-            %% the binary with the match context may not be safe. For example,
-            %% bs_match_SUITE:bin_tail_e/1 with inlining will be rejected by
-            %% beam_validator.
-            %%
-            %% Essentially the following code is produced:
-            %%
-            %% bs_match {x,0} => {x,0}
-            %% ...
-            %% bs_match {x,0} => {x,1}  %% ILLEGAL
-            %%
-            %% A bs_match instruction will only accept a match context as the
-            %% source operand if the source and destination registers are the
-            %% the same (as in the first bs_match instruction above).
-            %% The second bs_match instruction is therefore illegal.
-            %%
-            %% This situation is avoided if there is a stack frame:
-            %%
-            %% move {x,0} => {y,0}
-            %% bs_match {x,0} => {x,0}
-            %% ...
-            %% bs_match {y,0} => {x,1}  %% LEGAL
-            %%
-            throw(impossible)
-    end;
-avoid_stack_frame_1(#k_seq{arg=#k_call{anno=Anno,op=Op}=Call,
-                           body=#k_break{args=BrArgs0}}=Seq) ->
-    case Op of
-        #k_remote{mod=#k_atom{val=Mod},
-                  name=#k_atom{val=Name},
-                  arity=Arity} ->
-            case erl_bifs:is_exit_bif(Mod, Name, Arity) of
-                false ->
-                    %% Will clobber X registers. Must have a stack frame.
-                    throw(impossible);
-                true ->
-                    %% The call to this BIF will never return. It is safe
-                    %% to suppress the stack frame.
-                    Bif = #k_bif{anno=Anno,
-                                 op=#k_internal{name=guard_error,arity=1},
-                                 args=[Call],ret=[]},
-                    BrArgs = lists:duplicate(length(BrArgs0), #k_nil{}),
-                    GB = #k_guard_break{anno=#k{us=[],ns=[],a=[]},args=BrArgs},
-                    Seq#k_seq{arg=Bif,body=GB}
-            end;
-        _ ->
-            %% Will clobber X registers. Must have a stack frame.
-            throw(impossible)
-    end;
-avoid_stack_frame_1(#k_seq{arg=A0,body=B0}=Seq) ->
-    A = avoid_stack_frame_1(A0),
-    B = avoid_stack_frame_1(B0),
-    Seq#k_seq{arg=A,body=B};
-avoid_stack_frame_1(#k_test{}=Test) ->
-    Test;
-avoid_stack_frame_1(#k_type_clause{values=Values0}=TC) ->
-    Values = avoid_stack_frame_list(Values0),
-    TC#k_type_clause{values=Values};
-avoid_stack_frame_1(#k_val_clause{body=B0}=VC) ->
-    B = avoid_stack_frame_1(B0),
-    VC#k_val_clause{body=B};
-avoid_stack_frame_1(_Body) ->
-    throw(impossible).
-
-avoid_stack_frame_list([H|T]) ->
-    [avoid_stack_frame_1(H)|avoid_stack_frame_list(T)];
-avoid_stack_frame_list([]) -> [].
-
-
 %% This pass creates beam format annotated with variable lifetime
 %% information.  Each thing is given an index and for each variable we
 %% store the first and last index for its occurrence.  The variable
diff --git a/lib/compiler/test/match_SUITE.erl b/lib/compiler/test/match_SUITE.erl
index e3f842b668..72e5356a8d 100644
--- a/lib/compiler/test/match_SUITE.erl
+++ b/lib/compiler/test/match_SUITE.erl
@@ -378,6 +378,13 @@ untuplify(Config) when is_list(Config) ->
     %% We do this to cover sys_core_fold:unalias_pat/1.
     {1,2,3,4,alias,{[1,2],{3,4},alias}} = untuplify_1([1,2], {3,4}, alias),
     error = untuplify_1([1,2], {3,4}, 42),
+
+    %% Test that a previous bug in v3_codegen is gone. (The sinking of
+    %% stack frames into only the case arms that needed them was not always
+    %% safe.)
+    [33, -1, -33, 1] = untuplify_2(32, 65),
+    {33, 1, -33, -1} = untuplify_2(65, 32),
+
     ok.
 
 untuplify_1(A, B, C) ->
@@ -390,6 +397,21 @@ untuplify_1(A, B, C) ->
 	    error
     end.
 
+untuplify_2(V1, V2) ->
+    {D1,D2,D3,D4} =
+        if V1 > V2 ->
+                %% The 1 value was overwritten by the value of V2-V1.
+                {V1-V2,  1,  V2-V1, -1};
+           true ->
+                {V2-V1, -1, V1-V2,  1}
+        end,
+    if
+        D2 > D4 ->
+            {D1, D2, D3, D4};
+        true ->
+            [D1, D2, D3, D4]
+    end.
+
 %% Coverage of beam_dead:shortcut_boolean_label/4.
 shortcut_boolean(Config) when is_list(Config) ->
     false = shortcut_boolean_1([0]),