[turbofan] Never generate loop exit phis for the accumulator

The accumulator should never be alive when jumping back to a loop
header, or jumping out of a loop. This means that as far as far as
TurboFan is concerned, we never need to create Phis or LoopExitValues
for the accumulator, as its value should not escape the loop.

For safety, this also augments the IsLivenessValid DCHECK in the
liveness analysis to check that the accumulator is not live in these
cases, and amends the bytecode analysis tests to kill the accumulator
where necessary to ensure this.

As a drive-by, added some comments to the more complex bytecode analysis
tests, since figuring out what they were for and how to fix them took a
non-trivial amount of time.

Change-Id: Idecf76a36681d724134c59768650c23cc6b0e9ef
Reviewed-on: https://chromium-review.googlesource.com/615168
Commit-Queue: Jaroslav Sevcik <jarin@chromium.org>
Reviewed-by: Ross McIlroy <rmcilroy@chromium.org>
Reviewed-by: Jaroslav Sevcik <jarin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#47388}

src/compiler/bytecode-analysis.cc

@@ -59,13 +59,6 @@ bool BytecodeLoopAssignments::ContainsLocal(int index) const {
   return bit_vector_->Contains(parameter_count_ + index);
 }
 
-bool BytecodeLoopAssignments::ContainsAccumulator() const {
-  // TODO(leszeks): This assumes the accumulator is always assigned. This is
-  // probably correct, but that assignment is also probably dead, so we should
-  // check liveness.
-  return true;
-}
-
 BytecodeAnalysis::BytecodeAnalysis(Handle<BytecodeArray> bytecode_array,
                                    Zone* zone, bool do_liveness_analysis)
     : bytecode_array_(bytecode_array),
@@ -206,8 +199,21 @@ void UpdateOutLiveness(Bytecode bytecode, BytecodeLivenessState& out_liveness,
         table->LookupRange(current_offset, &handler_context, nullptr);
 
     if (handler_offset != -1) {
+      bool was_accumulator_live = out_liveness.AccumulatorIsLive();
       out_liveness.Union(*liveness_map.GetInLiveness(handler_offset));
       out_liveness.MarkRegisterLive(handler_context);
+      if (!was_accumulator_live) {
+        // The accumulator is reset to the exception on entry into a handler,
+        // and so shouldn't be considered live coming out of this bytecode just
+        // because it's live coming into the handler. So, kill the accumulator
+        // if the handler is the only thing that made it live.
+        out_liveness.MarkAccumulatorDead();
+
+        // TODO(leszeks): Ideally the accumulator wouldn't be considered live at
+        // the start of the handler, but looking up if the current bytecode is
+        // the start of a handler is not free, so we should only do it if we
+        // decide it's necessary.
+      }
     }
   }
 }
@@ -548,6 +554,36 @@ bool BytecodeAnalysis::LivenessIsValid() {
     next_bytecode_in_liveness = liveness.in;
   }
 
+  // Ensure that the accumulator is not live when jumping out of a loop, or on
+  // the back-edge of a loop.
+  for (iterator.GoToStart(); iterator.IsValid() && invalid_offset == -1;
+       ++iterator) {
+    Bytecode bytecode = iterator.current_bytecode();
+    int current_offset = iterator.current_offset();
+    int loop_header = GetLoopOffsetFor(current_offset);
+
+    // We only care if we're inside a loop.
+    if (loop_header == -1) continue;
+
+    // We only care about jumps.
+    if (!Bytecodes::IsJump(bytecode)) continue;
+
+    int jump_target = iterator.GetJumpTargetOffset();
+
+    // If this is a forward jump to somewhere else in the same loop, ignore it.
+    if (Bytecodes::IsForwardJump(bytecode) &&
+        GetLoopOffsetFor(jump_target) == loop_header) {
+      continue;
+    }
+
+    // The accumulator must be dead at the start of the target of the jump.
+    if (liveness_map_.GetLiveness(jump_target).in->AccumulatorIsLive()) {
+      invalid_offset = jump_target;
+      which_invalid = 0;
+      break;
+    }
+  }
+
   if (invalid_offset != -1) {
     OFStream of(stderr);
     of << "Invalid liveness:" << std::endl;
@@ -582,15 +618,19 @@ bool BytecodeAnalysis::LivenessIsValid() {
         loop_indent--;
       }
       for (int i = 0; i < loop_indent; ++i) {
-        of << " | ";
+        of << "| ";
       }
       if (forward_iterator.current_bytecode() == Bytecode::kJumpLoop) {
-        of << " `-" << current_offset;
+        of << "`-";
       } else if (IsLoopHeader(current_offset)) {
-        of << " .>" << current_offset;
+        of << ".>";
         loop_indent++;
       }
-      forward_iterator.PrintTo(of) << std::endl;
+      forward_iterator.PrintTo(of);
+      if (Bytecodes::IsJump(forward_iterator.current_bytecode())) {
+        of << " (@" << forward_iterator.GetJumpTargetOffset() << ")";
+      }
+      of << std::endl;
 
       if (current_offset == invalid_offset) {
         // Underline the invalid liveness.
@@ -598,6 +638,9 @@ bool BytecodeAnalysis::LivenessIsValid() {
           for (int i = 0; i < in_liveness.length(); ++i) {
             of << '^';
           }
+          for (int i = 0; i < out_liveness.length() + 3; ++i) {
+            of << ' ';
+          }
         } else {
           for (int i = 0; i < in_liveness.length() + 3; ++i) {
             of << ' ';
@@ -610,7 +653,7 @@ bool BytecodeAnalysis::LivenessIsValid() {
         // Make sure to draw the loop indentation marks on this additional line.
         of << " : " << current_offset << " : ";
         for (int i = 0; i < loop_indent; ++i) {
-          of << " | ";
+          of << "| ";
         }
 
         of << std::endl;

src/compiler/bytecode-analysis.h

@@ -30,7 +30,6 @@ class V8_EXPORT_PRIVATE BytecodeLoopAssignments {
 
   bool ContainsParameter(int index) const;
   bool ContainsLocal(int index) const;
-  bool ContainsAccumulator() const;
 
   int parameter_count() const { return parameter_count_; }
   int local_count() const { return bit_vector_->length() - parameter_count_; }

src/compiler/bytecode-graph-builder.cc

@@ -310,11 +310,6 @@ void BytecodeGraphBuilder::Environment::PrepareForLoop(
     }
   }
 
-  if (assignments.ContainsAccumulator()) {
-    values_[accumulator_base()] =
-        builder()->NewPhi(1, values_[accumulator_base()], control);
-  }
-
   // Connect to the loop end.
   Node* terminate = builder()->graph()->NewNode(
       builder()->common()->Terminate(), effect, control);
@@ -385,12 +380,6 @@ void BytecodeGraphBuilder::Environment::PrepareForLoopExit(
       values_[register_base() + i] = rename;
     }
   }
-
-  if (assignments.ContainsAccumulator()) {
-    Node* rename = graph()->NewNode(common()->LoopExitValue(),
-                                    values_[accumulator_base()], loop_exit);
-    values_[accumulator_base()] = rename;
-  }
 }
 
 void BytecodeGraphBuilder::Environment::UpdateStateValues(Node** state_values,

src/interpreter/bytecode-generator.cc

@@ -124,6 +124,10 @@ class BytecodeGenerator::ControlScope BASE_EMBEDDED {
     CMD_ASYNC_RETURN,
     CMD_RETHROW
   };
+  static constexpr bool CommandUsesAccumulator(Command command) {
+    return command != CMD_BREAK && command != CMD_CONTINUE;
+  }
+
   void PerformCommand(Command command, Statement* statement,
                       int source_position);
   virtual bool Execute(Command command, Statement* statement,
@@ -180,7 +184,9 @@ class BytecodeGenerator::ControlScope::DeferredCommands final {
     DCHECK_EQ(deferred_[token].statement, statement);
     DCHECK_EQ(deferred_[token].token, token);
 
-    builder()->StoreAccumulatorInRegister(result_register_);
+    if (CommandUsesAccumulator(command)) {
+      builder()->StoreAccumulatorInRegister(result_register_);
+    }
     builder()->LoadLiteral(Smi::FromInt(token));
     builder()->StoreAccumulatorInRegister(token_register_);
   }
@@ -217,7 +223,9 @@ class BytecodeGenerator::ControlScope::DeferredCommands final {
           .CompareOperation(Token::EQ_STRICT, token_register_)
           .JumpIfFalse(ToBooleanMode::kAlreadyBoolean, &fall_through);
 
-      builder()->LoadAccumulatorWithRegister(result_register_);
+      if (CommandUsesAccumulator(entry.command)) {
+        builder()->LoadAccumulatorWithRegister(result_register_);
+      }
       execution_control()->PerformCommand(entry.command, entry.statement,
                                           kNoSourcePosition);
     } else {
@@ -231,9 +239,11 @@ class BytecodeGenerator::ControlScope::DeferredCommands final {
           .SwitchOnSmiNoFeedback(jump_table)
           .Jump(&fall_through);
       for (const Entry& entry : deferred_) {
-        builder()
+        builder()->Bind(jump_table, entry.token);
-            ->Bind(jump_table, entry.token)
+
-            .LoadAccumulatorWithRegister(result_register_);
+        if (CommandUsesAccumulator(entry.command)) {
+          builder()->LoadAccumulatorWithRegister(result_register_);
+        }
         execution_control()->PerformCommand(entry.command, entry.statement,
                                             kNoSourcePosition);
       }

test/unittests/compiler/bytecode-analysis-unittest.cc

@@ -232,28 +232,35 @@ TEST_F(BytecodeAnalysisTest, SimpleLoop) {
   interpreter::Register reg_1(1);
   interpreter::Register reg_2(2);
 
+  // Kill r0.
   builder.StoreAccumulatorInRegister(reg_0);
-  expected_liveness.emplace_back("..LL", "L.LL");
+  expected_liveness.emplace_back("..LL", "L.L.");
 
   {
     interpreter::LoopBuilder loop_builder(&builder, nullptr, nullptr);
     loop_builder.LoopHeader();
 
+    builder.LoadUndefined();
+    expected_liveness.emplace_back("L.L.", "L.LL");
+
     builder.JumpIfTrue(ToBooleanMode::kConvertToBoolean,
                        loop_builder.break_labels()->New());
     expected_liveness.emplace_back("L.LL", "L.L.");
 
+    // Gen r0.
     builder.LoadAccumulatorWithRegister(reg_0);
     expected_liveness.emplace_back("L...", "L..L");
 
+    // Kill r2.
     builder.StoreAccumulatorInRegister(reg_2);
-    expected_liveness.emplace_back("L..L", "L.LL");
+    expected_liveness.emplace_back("L..L", "L.L.");
 
     loop_builder.BindContinueTarget();
     loop_builder.JumpToHeader(0);
-    expected_liveness.emplace_back("L.LL", "L.LL");
+    expected_liveness.emplace_back("L.L.", "L.L.");
   }
 
+  // Gen r2.
   builder.LoadAccumulatorWithRegister(reg_2);
   expected_liveness.emplace_back("..L.", "...L");
 
@@ -273,15 +280,18 @@ TEST_F(BytecodeAnalysisTest, TryCatch) {
   interpreter::Register reg_1(1);
   interpreter::Register reg_context(2);
 
+  // Kill r0.
   builder.StoreAccumulatorInRegister(reg_0);
   expected_liveness.emplace_back(".LLL", "LLL.");
 
   interpreter::TryCatchBuilder try_builder(&builder, HandlerTable::CAUGHT);
   try_builder.BeginTry(reg_context);
   {
+    // Gen r0.
     builder.LoadAccumulatorWithRegister(reg_0);
     expected_liveness.emplace_back("LLL.", ".LLL");
 
+    // Kill r0.
     builder.StoreAccumulatorInRegister(reg_0);
     expected_liveness.emplace_back(".LLL", ".LL.");
 
@@ -311,20 +321,21 @@ TEST_F(BytecodeAnalysisTest, TryCatch) {
 }
 
 TEST_F(BytecodeAnalysisTest, DiamondInLoop) {
+  // For a logic diamond inside a loop, the liveness down one path of the
+  // diamond should eventually propagate up the other path when the loop is
+  // reprocessed.
+
   interpreter::BytecodeArrayBuilder builder(isolate(), zone(), 3, 3);
   std::vector<std::pair<std::string, std::string>> expected_liveness;
 
   interpreter::Register reg_0(0);
-  interpreter::Register reg_1(1);
-  interpreter::Register reg_2(2);
-
-  builder.StoreAccumulatorInRegister(reg_0);
-  expected_liveness.emplace_back("...L", "L..L");
 
   {
     interpreter::LoopBuilder loop_builder(&builder, nullptr, nullptr);
     loop_builder.LoopHeader();
 
+    builder.LoadUndefined();
+    expected_liveness.emplace_back("L...", "L..L");
     builder.JumpIfTrue(ToBooleanMode::kConvertToBoolean,
                        loop_builder.break_labels()->New());
     expected_liveness.emplace_back("L..L", "L..L");
@@ -332,26 +343,29 @@ TEST_F(BytecodeAnalysisTest, DiamondInLoop) {
     interpreter::BytecodeLabel ld1_label;
     interpreter::BytecodeLabel end_label;
     builder.JumpIfTrue(ToBooleanMode::kConvertToBoolean, &ld1_label);
-    expected_liveness.emplace_back("L..L", "L..L");
+    expected_liveness.emplace_back("L..L", "L...");
 
     {
       builder.Jump(&end_label);
-      expected_liveness.emplace_back("L..L", "L..L");
+      expected_liveness.emplace_back("L...", "L...");
     }
 
     builder.Bind(&ld1_label);
     {
+      // Gen r0.
       builder.LoadAccumulatorWithRegister(reg_0);
-      expected_liveness.emplace_back("L...", "L..L");
+      expected_liveness.emplace_back("L...", "L...");
     }
 
     builder.Bind(&end_label);
 
     loop_builder.BindContinueTarget();
     loop_builder.JumpToHeader(0);
-    expected_liveness.emplace_back("L..L", "L..L");
+    expected_liveness.emplace_back("L...", "L...");
   }
 
+  builder.LoadUndefined();
+  expected_liveness.emplace_back("....", "...L");
   builder.Return();
   expected_liveness.emplace_back("...L", "....");
 
@@ -361,43 +375,65 @@ TEST_F(BytecodeAnalysisTest, DiamondInLoop) {
 }
 
 TEST_F(BytecodeAnalysisTest, KillingLoopInsideLoop) {
+  // For a loop inside a loop, the inner loop has to be processed after the
+  // outer loop has been processed, to ensure that it can propagate the
+  // information in its header. Consider
+  //
+  //     0: do {
+  //     1:   acc = r0;
+  //     2:   acc = r1;
+  //     3:   do {
+  //     4:     r0 = acc;
+  //     5:     break;
+  //     6:   } while(true);
+  //     7: } while(true);
+  //
+  // r0 should should be dead at 3 and 6, while r1 is live throughout. On the
+  // initial pass, r1 is dead from 3-7. On the outer loop pass, it becomes live
+  // in 3 and 7 (but not 4-6 because 6 only reads liveness from 3). Only after
+  // the inner loop pass does it become live in 4-6. It's necessary, however, to
+  // still process the inner loop when processing the outer loop, to ensure that
+  // r1 becomes live in 3 (via 5), but r0 stays dead (because of 4).
+
   interpreter::BytecodeArrayBuilder builder(isolate(), zone(), 3, 3);
   std::vector<std::pair<std::string, std::string>> expected_liveness;
 
   interpreter::Register reg_0(0);
   interpreter::Register reg_1(1);
 
-  builder.StoreAccumulatorInRegister(reg_0);
-  expected_liveness.emplace_back(".L.L", "LL..");
-
   {
     interpreter::LoopBuilder loop_builder(&builder, nullptr, nullptr);
     loop_builder.LoopHeader();
 
+    // Gen r0.
     builder.LoadAccumulatorWithRegister(reg_0);
     expected_liveness.emplace_back("LL..", ".L..");
 
+    // Gen r1.
     builder.LoadAccumulatorWithRegister(reg_1);
     expected_liveness.emplace_back(".L..", ".L.L");
 
     builder.JumpIfTrue(ToBooleanMode::kConvertToBoolean,
                        loop_builder.break_labels()->New());
-    expected_liveness.emplace_back(".L.L", ".L.L");
+    expected_liveness.emplace_back(".L.L", ".L..");
 
     {
       interpreter::LoopBuilder inner_loop_builder(&builder, nullptr, nullptr);
       inner_loop_builder.LoopHeader();
 
+      // Kill r0.
+      builder.LoadUndefined();
+      expected_liveness.emplace_back(".L..", ".L.L");
       builder.StoreAccumulatorInRegister(reg_0);
       expected_liveness.emplace_back(".L.L", "LL.L");
 
       builder.JumpIfTrue(ToBooleanMode::kConvertToBoolean,
                          inner_loop_builder.break_labels()->New());
-      expected_liveness.emplace_back("LL.L", "LL.L");
+      expected_liveness.emplace_back("LL.L", "LL..");
 
       inner_loop_builder.BindContinueTarget();
       inner_loop_builder.JumpToHeader(1);
-      expected_liveness.emplace_back(".L.L", ".L.L");
+      expected_liveness.emplace_back(".L..", ".L..");
     }
 
     loop_builder.BindContinueTarget();
@@ -405,6 +441,8 @@ TEST_F(BytecodeAnalysisTest, KillingLoopInsideLoop) {
     expected_liveness.emplace_back("LL..", "LL..");
   }
 
+  builder.LoadUndefined();
+  expected_liveness.emplace_back("....", "...L");
   builder.Return();
   expected_liveness.emplace_back("...L", "....");