[wasm][liftoff] Optimize one-armed ifs

Do not implement one-armed ifs by emulating an empty else branch. In
Liftoff, we can generate better code and save compile time by handling
this specially. If the merge point at the end of the if is not reached
by the if-branch, we do not need to generate any merge code.

R=titzer@chromium.org

Bug: v8:6600, v8:8423
Change-Id: Ie8ea69dd7491f225605a8e1b986d275d869aa90b
Reviewed-on: https://chromium-review.googlesource.com/c/1356508
Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
Reviewed-by: Ben Titzer <titzer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#57968}

src/wasm/baseline/liftoff-compiler.cc

@@ -498,8 +498,10 @@ class LiftoffCompiler {
     if (c->end_merge.reached) {
       __ MergeFullStackWith(c->label_state);
     } else if (c->is_onearmed_if()) {
-      c->label_state.InitMerge(*__ cache_state(), __ num_locals(),
-                               c->br_merge()->arity);
+      // Init the merge point from the else state, then merge the if state into
+      // that.
+      DCHECK_EQ(0, c->end_merge.arity);
+      c->label_state.InitMerge(c->else_state->state, __ num_locals(), 0);
       __ MergeFullStackWith(c->label_state);
     } else {
       c->label_state.Split(*__ cache_state());
@@ -508,7 +510,20 @@ class LiftoffCompiler {
   }
 
   void PopControl(FullDecoder* decoder, Control* c) {
-    if (!c->is_loop() && c->end_merge.reached) {
+    if (c->is_onearmed_if()) {
+      __ bind(c->else_state->label.get());
+      if (c->end_merge.reached) {
+        // Generate the code to merge the else state into the end state.
+        // TODO(clemensh): Do this without switching to the else state first.
+        __ cache_state()->Steal(c->else_state->state);
+        __ MergeFullStackWith(c->label_state);
+        __ cache_state()->Steal(c->label_state);
+      } else {
+        // There is no merge at the end of the if, so just continue with the
+        // else state.
+        __ cache_state()->Steal(c->else_state->state);
+      }
+    } else if (!c->is_loop() && c->end_merge.reached) {
       __ cache_state()->Steal(c->label_state);
     }
     if (!c->label.get()->is_bound()) {
@@ -1318,10 +1333,17 @@ class LiftoffCompiler {
     DCHECK(!table_iterator.has_next());
   }
 
-  void Else(FullDecoder* decoder, Control* if_block) {
-    if (if_block->reachable()) __ emit_jump(if_block->label.get());
-    __ bind(if_block->else_state->label.get());
-    __ cache_state()->Steal(if_block->else_state->state);
+  void Else(FullDecoder* decoder, Control* c) {
+    if (c->reachable()) {
+      if (!c->end_merge.reached) {
+        c->label_state.InitMerge(*__ cache_state(), __ num_locals(),
+                                 c->end_merge.arity);
+      }
+      __ MergeFullStackWith(c->label_state);
+      __ emit_jump(c->label.get());
+    }
+    __ bind(c->else_state->label.get());
+    __ cache_state()->Steal(c->else_state->state);
   }
 
   Label* AddOutOfLineTrap(WasmCodePosition position,

src/wasm/function-body-decoder-impl.h

@@ -1757,9 +1757,10 @@ class WasmFullDecoder : public WasmDecoder<validate> {
               this->error(this->pc_, "else already present for if");
               break;
             }
-            FallThruTo(c);
+            if (!TypeCheckFallThru(c)) break;
             c->kind = kControlIfElse;
             CALL_INTERFACE_IF_PARENT_REACHABLE(Else, c);
+            if (c->reachable()) c->end_merge.reached = true;
             PushMergeValues(c, &c->start_merge);
             c->reachability = control_at(1)->innerReachability();
             break;
@@ -1767,7 +1768,7 @@ class WasmFullDecoder : public WasmDecoder<validate> {
           case kExprEnd: {
             if (!VALIDATE(!control_.empty())) {
               this->error("end does not match any if, try, or block");
-              return;
+              break;
             }
             Control* c = &control_.back();
             if (!VALIDATE(!c->is_incomplete_try())) {
@@ -1775,12 +1776,12 @@ class WasmFullDecoder : public WasmDecoder<validate> {
               break;
             }
             if (c->is_onearmed_if()) {
-              // Emulate empty else arm.
-              FallThruTo(c);
-              if (this->failed()) break;
-              CALL_INTERFACE_IF_PARENT_REACHABLE(Else, c);
-              PushMergeValues(c, &c->start_merge);
-              c->reachability = control_at(1)->innerReachability();
+              if (!VALIDATE(c->end_merge.arity == c->start_merge.arity)) {
+                this->error(
+                    c->pc,
+                    "start-arity and end-arity of one-armed if must match");
+                break;
+              }
             }
             if (c->is_try_catch()) {
               // Emulate catch-all + re-throw.
@@ -2310,7 +2311,7 @@ class WasmFullDecoder : public WasmDecoder<validate> {
   void PopControl(Control* c) {
     DCHECK_EQ(c, &control_.back());
     CALL_INTERFACE_IF_PARENT_REACHABLE(PopControl, c);
-    bool reached = c->end_merge.reached;
+    bool reached = c->end_merge.reached || c->is_onearmed_if();
     control_.pop_back();
     // If the parent block was reachable before, but the popped control does not
     // return to here, this block becomes indirectly unreachable.

src/wasm/graph-builder-interface.cc

@@ -200,6 +200,11 @@ class WasmGraphBuildingInterface {
   }
 
   void PopControl(FullDecoder* decoder, Control* block) {
+    if (block->is_onearmed_if()) {
+      // Merge the else branch into the end merge.
+      SetEnv(block->false_env);
+      MergeValuesInto(decoder, block, &block->end_merge);
+    }
     if (!block->is_loop()) SetEnv(block->end_env);
   }
 
@@ -338,6 +343,10 @@ class WasmGraphBuildingInterface {
   }
 
   void Else(FullDecoder* decoder, Control* if_block) {
+    if (if_block->reachable()) {
+      // Merge the if branch into the end merge.
+      MergeValuesInto(decoder, if_block, &if_block->end_merge);
+    }
     SetEnv(if_block->false_env);
   }
 

test/unittests/wasm/function-body-decoder-unittest.cc

@@ -18,6 +18,7 @@
 #include "test/common/wasm/flag-utils.h"
 #include "test/common/wasm/test-signatures.h"
 #include "test/common/wasm/wasm-macro-gen.h"
+#include "testing/gmock-support.h"
 
 namespace v8 {
 namespace internal {
@@ -146,6 +147,9 @@ class FunctionBodyDecoderTest : public TestWithZone {
       str << "Verification successed, expected failure; pc = +" << pc;
     }
     EXPECT_EQ(result.ok(), expected_success) << str.str();
+    if (!expected_success && message) {
+      EXPECT_THAT(result.error_msg(), ::testing::HasSubstr(message));
+    }
   }
 
   void TestBinop(WasmOpcode opcode, FunctionSig* success) {
@@ -786,6 +790,13 @@ TEST_F(FunctionBodyDecoderTest, IfElseUnreachable2) {
   }
 }
 
+TEST_F(FunctionBodyDecoderTest, OneArmedIfWithArity) {
+  static const byte code[] = {WASM_ZERO, kExprIf, kLocalI32, WASM_ONE,
+                              kExprEnd};
+  EXPECT_FAILURE_C(i_v, code,
+                   "start-arity and end-arity of one-armed if must match");
+}
+
 TEST_F(FunctionBodyDecoderTest, IfBreak) {
   EXPECT_VERIFIES(v_i, WASM_IF(WASM_GET_LOCAL(0), WASM_BR(0)));
   EXPECT_VERIFIES(v_i, WASM_IF(WASM_GET_LOCAL(0), WASM_BR(1)));