[interpreter] Add accumulator use description to bytecodes.

Anotates bytecodes with a description of how each uses the accumulator.

Validates annotations and uses of accumulator when generating bytecode
handlers.

Only prints the accumulator during tracing where used.

BUG=v8:4280
LOG=N

Review URL: https://codereview.chromium.org/1852213002

Cr-Commit-Position: refs/heads/master@{#35281}

src/interpreter/bytecode-traits.h

@@ -11,11 +11,6 @@ namespace v8 {
 namespace internal {
 namespace interpreter {
 
-// TODO(rmcilroy): consider simplifying this to avoid the template magic.
-
-// Template helpers to deduce the number of operands each bytecode has.
-#define OPERAND_TERM OperandType::kNone, OperandType::kNone, OperandType::kNone
-
 template <OperandTypeInfo>
 struct OperandTypeInfoTraits {
   static const bool kIsScalable = false;
@@ -59,13 +54,13 @@ struct RegisterOperandTraits {
 REGISTER_OPERAND_TYPE_LIST(DECLARE_REGISTER_OPERAND)
 #undef DECLARE_REGISTER_OPERAND
 
-template <OperandType... Args>
+template <AccumulatorUse, OperandType...>
 struct BytecodeTraits {};
 
-template <OperandType operand_0, OperandType operand_1, OperandType operand_2,
-          OperandType operand_3>
-struct BytecodeTraits<operand_0, operand_1, operand_2, operand_3,
-                      OPERAND_TERM> {
+template <AccumulatorUse accumulator_use, OperandType operand_0,
+          OperandType operand_1, OperandType operand_2, OperandType operand_3>
+struct BytecodeTraits<accumulator_use, operand_0, operand_1, operand_2,
+                      operand_3> {
   static OperandType GetOperandType(int i) {
     DCHECK(0 <= i && i < kOperandCount);
     const OperandType kOperands[] = {operand_0, operand_1, operand_2,
@@ -86,6 +81,7 @@ struct BytecodeTraits<operand_0, operand_1, operand_2, operand_3,
             OperandTraits<operand_3>::TypeInfo::kIsScalable);
   }
 
+  static const AccumulatorUse kAccumulatorUse = accumulator_use;
   static const int kOperandCount = 4;
   static const int kRegisterOperandCount =
       RegisterOperandTraits<operand_0>::kIsRegisterOperand +
@@ -99,8 +95,9 @@ struct BytecodeTraits<operand_0, operand_1, operand_2, operand_3,
       (RegisterOperandTraits<operand_3>::kIsRegisterOperand << 3);
 };
 
-template <OperandType operand_0, OperandType operand_1, OperandType operand_2>
-struct BytecodeTraits<operand_0, operand_1, operand_2, OPERAND_TERM> {
+template <AccumulatorUse accumulator_use, OperandType operand_0,
+          OperandType operand_1, OperandType operand_2>
+struct BytecodeTraits<accumulator_use, operand_0, operand_1, operand_2> {
   static inline OperandType GetOperandType(int i) {
     DCHECK(0 <= i && i <= 2);
     const OperandType kOperands[] = {operand_0, operand_1, operand_2};
@@ -118,6 +115,7 @@ struct BytecodeTraits<operand_0, operand_1, operand_2, OPERAND_TERM> {
             OperandTraits<operand_2>::TypeInfo::kIsScalable);
   }
 
+  static const AccumulatorUse kAccumulatorUse = accumulator_use;
   static const int kOperandCount = 3;
   static const int kRegisterOperandCount =
       RegisterOperandTraits<operand_0>::kIsRegisterOperand +
@@ -129,8 +127,9 @@ struct BytecodeTraits<operand_0, operand_1, operand_2, OPERAND_TERM> {
       (RegisterOperandTraits<operand_2>::kIsRegisterOperand << 2);
 };
 
-template <OperandType operand_0, OperandType operand_1>
-struct BytecodeTraits<operand_0, operand_1, OPERAND_TERM> {
+template <AccumulatorUse accumulator_use, OperandType operand_0,
+          OperandType operand_1>
+struct BytecodeTraits<accumulator_use, operand_0, operand_1> {
   static inline OperandType GetOperandType(int i) {
     DCHECK(0 <= i && i < kOperandCount);
     const OperandType kOperands[] = {operand_0, operand_1};
@@ -147,6 +146,7 @@ struct BytecodeTraits<operand_0, operand_1, OPERAND_TERM> {
             OperandTraits<operand_1>::TypeInfo::kIsScalable);
   }
 
+  static const AccumulatorUse kAccumulatorUse = accumulator_use;
   static const int kOperandCount = 2;
   static const int kRegisterOperandCount =
       RegisterOperandTraits<operand_0>::kIsRegisterOperand +
@@ -156,8 +156,8 @@ struct BytecodeTraits<operand_0, operand_1, OPERAND_TERM> {
       (RegisterOperandTraits<operand_1>::kIsRegisterOperand << 1);
 };
 
-template <OperandType operand_0>
-struct BytecodeTraits<operand_0, OPERAND_TERM> {
+template <AccumulatorUse accumulator_use, OperandType operand_0>
+struct BytecodeTraits<accumulator_use, operand_0> {
   static inline OperandType GetOperandType(int i) {
     DCHECK(i == 0);
     return operand_0;
@@ -172,6 +172,7 @@ struct BytecodeTraits<operand_0, OPERAND_TERM> {
     return OperandTraits<operand_0>::TypeInfo::kIsScalable;
   }
 
+  static const AccumulatorUse kAccumulatorUse = accumulator_use;
   static const int kOperandCount = 1;
   static const int kRegisterOperandCount =
       RegisterOperandTraits<operand_0>::kIsRegisterOperand;
@@ -179,8 +180,8 @@ struct BytecodeTraits<operand_0, OPERAND_TERM> {
       RegisterOperandTraits<operand_0>::kIsRegisterOperand;
 };
 
-template <>
-struct BytecodeTraits<OperandType::kNone, OPERAND_TERM> {
+template <AccumulatorUse accumulator_use>
+struct BytecodeTraits<accumulator_use> {
   static inline OperandType GetOperandType(int i) {
     UNREACHABLE();
     return OperandType::kNone;
@@ -193,6 +194,7 @@ struct BytecodeTraits<OperandType::kNone, OPERAND_TERM> {
 
   static inline bool IsScalable() { return false; }
 
+  static const AccumulatorUse kAccumulatorUse = accumulator_use;
   static const int kOperandCount = 0;
   static const int kRegisterOperandCount = 0;
   static const int kRegisterOperandBitmap = 0;

src/interpreter/bytecodes.cc

@@ -42,6 +42,22 @@ std::string Bytecodes::ToString(Bytecode bytecode, OperandScale operand_scale) {
   }
 }
 
+// static
+const char* Bytecodes::AccumulatorUseToString(AccumulatorUse accumulator_use) {
+  switch (accumulator_use) {
+    case AccumulatorUse::kNone:
+      return "None";
+    case AccumulatorUse::kRead:
+      return "Read";
+    case AccumulatorUse::kWrite:
+      return "Write";
+    case AccumulatorUse::kReadWrite:
+      return "ReadWrite";
+  }
+  UNREACHABLE();
+  return "";
+}
+
 // static
 const char* Bytecodes::OperandTypeToString(OperandType operand_type) {
   switch (operand_type) {
@@ -141,7 +157,7 @@ int Bytecodes::NumberOfOperands(Bytecode bytecode) {
   switch (bytecode) {
 #define CASE(Name, ...)   \
   case Bytecode::k##Name: \
-    return BytecodeTraits<__VA_ARGS__, OPERAND_TERM>::kOperandCount;
+    return BytecodeTraits<__VA_ARGS__>::kOperandCount;
     BYTECODE_LIST(CASE)
 #undef CASE
   }
@@ -154,9 +170,9 @@ int Bytecodes::NumberOfOperands(Bytecode bytecode) {
 int Bytecodes::NumberOfRegisterOperands(Bytecode bytecode) {
   DCHECK(bytecode <= Bytecode::kLast);
   switch (bytecode) {
-#define CASE(Name, ...)                                            \
-  case Bytecode::k##Name:                                          \
-    typedef BytecodeTraits<__VA_ARGS__, OPERAND_TERM> Name##Trait; \
+#define CASE(Name, ...)                              \
+  case Bytecode::k##Name:                            \
+    typedef BytecodeTraits<__VA_ARGS__> Name##Trait; \
     return Name##Trait::kRegisterOperandCount;
     BYTECODE_LIST(CASE)
 #undef CASE
@@ -198,13 +214,39 @@ OperandScale Bytecodes::PrefixBytecodeToOperandScale(Bytecode bytecode) {
   }
 }
 
+// static
+AccumulatorUse Bytecodes::GetAccumulatorUse(Bytecode bytecode) {
+  DCHECK(bytecode <= Bytecode::kLast);
+  switch (bytecode) {
+#define CASE(Name, ...)   \
+  case Bytecode::k##Name: \
+    return BytecodeTraits<__VA_ARGS__>::kAccumulatorUse;
+    BYTECODE_LIST(CASE)
+#undef CASE
+  }
+  UNREACHABLE();
+  return AccumulatorUse::kNone;
+}
+
+// static
+bool Bytecodes::ReadsAccumulator(Bytecode bytecode) {
+  return (GetAccumulatorUse(bytecode) & AccumulatorUse::kRead) ==
+         AccumulatorUse::kRead;
+}
+
+// static
+bool Bytecodes::WritesAccumulator(Bytecode bytecode) {
+  return (GetAccumulatorUse(bytecode) & AccumulatorUse::kWrite) ==
+         AccumulatorUse::kWrite;
+}
+
 // static
 OperandType Bytecodes::GetOperandType(Bytecode bytecode, int i) {
   DCHECK(bytecode <= Bytecode::kLast);
   switch (bytecode) {
 #define CASE(Name, ...)   \
   case Bytecode::k##Name: \
-    return BytecodeTraits<__VA_ARGS__, OPERAND_TERM>::GetOperandType(i);
+    return BytecodeTraits<__VA_ARGS__>::GetOperandType(i);
     BYTECODE_LIST(CASE)
 #undef CASE
   }
@@ -223,9 +265,9 @@ OperandSize Bytecodes::GetOperandSize(Bytecode bytecode, int i,
 int Bytecodes::GetRegisterOperandBitmap(Bytecode bytecode) {
   DCHECK(bytecode <= Bytecode::kLast);
   switch (bytecode) {
-#define CASE(Name, ...)                                            \
-  case Bytecode::k##Name:                                          \
-    typedef BytecodeTraits<__VA_ARGS__, OPERAND_TERM> Name##Trait; \
+#define CASE(Name, ...)                              \
+  case Bytecode::k##Name:                            \
+    typedef BytecodeTraits<__VA_ARGS__> Name##Trait; \
     return Name##Trait::kRegisterOperandBitmap;
     BYTECODE_LIST(CASE)
 #undef CASE
@@ -331,9 +373,9 @@ bool Bytecodes::IsDebugBreak(Bytecode bytecode) {
 // static
 bool Bytecodes::IsBytecodeWithScalableOperands(Bytecode bytecode) {
   switch (bytecode) {
-#define CASE(Name, ...)                                            \
-  case Bytecode::k##Name:                                          \
-    typedef BytecodeTraits<__VA_ARGS__, OPERAND_TERM> Name##Trait; \
+#define CASE(Name, ...)                              \
+  case Bytecode::k##Name:                            \
+    typedef BytecodeTraits<__VA_ARGS__> Name##Trait; \
     return Name##Trait::IsScalable();
     BYTECODE_LIST(CASE)
 #undef CASE
@@ -586,6 +628,10 @@ std::ostream& operator<<(std::ostream& os, const Bytecode& bytecode) {
   return os << Bytecodes::ToString(bytecode);
 }
 
+std::ostream& operator<<(std::ostream& os, const AccumulatorUse& use) {
+  return os << Bytecodes::AccumulatorUseToString(use);
+}
+
 std::ostream& operator<<(std::ostream& os, const OperandSize& operand_size) {
   return os << Bytecodes::OperandSizeToString(operand_size);
 }

src/interpreter/interpreter-assembler.cc

@@ -31,6 +31,7 @@ InterpreterAssembler::InterpreterAssembler(Isolate* isolate, Zone* zone,
       bytecode_(bytecode),
       operand_scale_(operand_scale),
       accumulator_(this, MachineRepresentation::kTagged),
+      accumulator_use_(AccumulatorUse::kNone),
       context_(this, MachineRepresentation::kTagged),
       bytecode_array_(this, MachineRepresentation::kTagged),
       disable_stack_check_across_call_(false),
@@ -45,11 +46,26 @@ InterpreterAssembler::InterpreterAssembler(Isolate* isolate, Zone* zone,
   }
 }
 
-InterpreterAssembler::~InterpreterAssembler() {}
+InterpreterAssembler::~InterpreterAssembler() {
+  // If the following check fails the handler does not use the
+  // accumulator in the way described in the bytecode definitions in
+  // bytecodes.h.
+  DCHECK_EQ(accumulator_use_, Bytecodes::GetAccumulatorUse(bytecode_));
+}
+
+Node* InterpreterAssembler::GetAccumulatorUnchecked() {
+  return accumulator_.value();
+}
 
-Node* InterpreterAssembler::GetAccumulator() { return accumulator_.value(); }
+Node* InterpreterAssembler::GetAccumulator() {
+  DCHECK(Bytecodes::ReadsAccumulator(bytecode_));
+  accumulator_use_ = accumulator_use_ | AccumulatorUse::kRead;
+  return GetAccumulatorUnchecked();
+}
 
 void InterpreterAssembler::SetAccumulator(Node* value) {
+  DCHECK(Bytecodes::WritesAccumulator(bytecode_));
+  accumulator_use_ = accumulator_use_ | AccumulatorUse::kWrite;
   accumulator_.Bind(value);
 }
 
@@ -554,7 +570,7 @@ void InterpreterAssembler::DispatchToBytecodeHandler(Node* handler,
   }
 
   InterpreterDispatchDescriptor descriptor(isolate());
-  Node* args[] = {GetAccumulator(),          RegisterFileRawPointer(),
+  Node* args[] = {GetAccumulatorUnchecked(), RegisterFileRawPointer(),
                   bytecode_offset,           BytecodeArrayTaggedPointer(),
                   DispatchTableRawPointer(), GetContext()};
   TailCall(descriptor, handler, args, 0);
@@ -654,7 +670,7 @@ void InterpreterAssembler::AbortIfWordNotEqual(Node* lhs, Node* rhs,
 
 void InterpreterAssembler::TraceBytecode(Runtime::FunctionId function_id) {
   CallRuntime(function_id, GetContext(), BytecodeArrayTaggedPointer(),
-              SmiTag(BytecodeOffset()), GetAccumulator());
+              SmiTag(BytecodeOffset()), GetAccumulatorUnchecked());
 }
 
 // static

src/interpreter/interpreter-assembler.h

@@ -150,6 +150,7 @@ class InterpreterAssembler : public compiler::CodeStubAssembler {
   void Abort(BailoutReason bailout_reason);
 
  protected:
+  Bytecode bytecode() const { return bytecode_; }
   static bool TargetSupportsUnalignedAccess();
 
  private:
@@ -162,6 +163,11 @@ class InterpreterAssembler : public compiler::CodeStubAssembler {
   // Returns a raw pointer to first entry in the interpreter dispatch table.
   compiler::Node* DispatchTableRawPointer();
 
+  // Returns the accumulator value without checking whether bytecode
+  // uses it. This is intended to be used only in dispatch and in
+  // tracing as these need to bypass accumulator use validity checks.
+  compiler::Node* GetAccumulatorUnchecked();
+
   // Saves and restores interpreter bytecode offset to the interpreter stack
   // frame when performing a call.
   void CallPrologue() override;
@@ -217,6 +223,7 @@ class InterpreterAssembler : public compiler::CodeStubAssembler {
   Bytecode bytecode_;
   OperandScale operand_scale_;
   CodeStubAssembler::Variable accumulator_;
+  AccumulatorUse accumulator_use_;
   CodeStubAssembler::Variable context_;
   CodeStubAssembler::Variable bytecode_array_;
 

src/runtime/runtime-interpreter.cc

@@ -44,7 +44,8 @@ void AdvanceToOffsetForTracing(
 void PrintRegisters(std::ostream& os, bool is_input,
                     interpreter::BytecodeArrayIterator& bytecode_iterator,
                     Handle<Object> accumulator) {
-  static const int kRegFieldWidth = static_cast<int>(strlen("accumulator"));
+  static const char kAccumulator[] = "accumulator";
+  static const int kRegFieldWidth = static_cast<int>(sizeof(kAccumulator) - 1);
   static const char* kInputColourCode = "\033[0;36m";
   static const char* kOutputColourCode = "\033[0;35m";
   static const char* kNormalColourCode = "\033[0;m";
@@ -53,10 +54,15 @@ void PrintRegisters(std::ostream& os, bool is_input,
     os << (is_input ? kInputColourCode : kOutputColourCode);
   }
 
+  interpreter::Bytecode bytecode = bytecode_iterator.current_bytecode();
+
   // Print accumulator.
-  os << "      [ accumulator" << kArrowDirection;
-  accumulator->ShortPrint();
-  os << " ]" << std::endl;
+  if ((is_input && interpreter::Bytecodes::ReadsAccumulator(bytecode)) ||
+      (!is_input && interpreter::Bytecodes::WritesAccumulator(bytecode))) {
+    os << "      [ " << kAccumulator << kArrowDirection;
+    accumulator->ShortPrint();
+    os << " ]" << std::endl;
+  }
 
   // Find the location of the register file.
   JavaScriptFrameIterator frame_iterator(
@@ -66,7 +72,6 @@ void PrintRegisters(std::ostream& os, bool is_input,
       frame->fp() + InterpreterFrameConstants::kRegisterFilePointerFromFp;
 
   // Print the registers.
-  interpreter::Bytecode bytecode = bytecode_iterator.current_bytecode();
   int operand_count = interpreter::Bytecodes::NumberOfOperands(bytecode);
   for (int operand_index = 0; operand_index < operand_count; operand_index++) {
     interpreter::OperandType operand_type =

test/unittests/interpreter/bytecodes-unittest.cc

@@ -280,6 +280,41 @@ TEST(OperandScale, PrefixesRequired) {
         Bytecode::kExtraWide);
 }
 
+TEST(AccumulatorUse, LogicalOperators) {
+  CHECK_EQ(AccumulatorUse::kNone | AccumulatorUse::kRead,
+           AccumulatorUse::kRead);
+  CHECK_EQ(AccumulatorUse::kRead | AccumulatorUse::kWrite,
+           AccumulatorUse::kReadWrite);
+  CHECK_EQ(AccumulatorUse::kRead & AccumulatorUse::kReadWrite,
+           AccumulatorUse::kRead);
+  CHECK_EQ(AccumulatorUse::kRead & AccumulatorUse::kWrite,
+           AccumulatorUse::kNone);
+}
+
+TEST(AccumulatorUse, SampleBytecodes) {
+  CHECK(Bytecodes::ReadsAccumulator(Bytecode::kStar));
+  CHECK(!Bytecodes::WritesAccumulator(Bytecode::kStar));
+  CHECK_EQ(Bytecodes::GetAccumulatorUse(Bytecode::kStar),
+           AccumulatorUse::kRead);
+  CHECK(!Bytecodes::ReadsAccumulator(Bytecode::kLdar));
+  CHECK(Bytecodes::WritesAccumulator(Bytecode::kLdar));
+  CHECK_EQ(Bytecodes::GetAccumulatorUse(Bytecode::kLdar),
+           AccumulatorUse::kWrite);
+  CHECK(Bytecodes::ReadsAccumulator(Bytecode::kAdd));
+  CHECK(Bytecodes::WritesAccumulator(Bytecode::kAdd));
+  CHECK_EQ(Bytecodes::GetAccumulatorUse(Bytecode::kAdd),
+           AccumulatorUse::kReadWrite);
+}
+
+TEST(AccumulatorUse, AccumulatorUseToString) {
+  std::set<std::string> names;
+  names.insert(Bytecodes::AccumulatorUseToString(AccumulatorUse::kNone));
+  names.insert(Bytecodes::AccumulatorUseToString(AccumulatorUse::kRead));
+  names.insert(Bytecodes::AccumulatorUseToString(AccumulatorUse::kWrite));
+  names.insert(Bytecodes::AccumulatorUseToString(AccumulatorUse::kReadWrite));
+  CHECK_EQ(names.size(), 4);
+}
+
 }  // namespace interpreter
 }  // namespace internal
 }  // namespace v8

test/unittests/interpreter/interpreter-assembler-unittest.cc

@@ -62,6 +62,18 @@ Matcher<Node*> IsWordOr(const Matcher<Node*>& lhs_matcher,
                            : IsWord32Or(lhs_matcher, rhs_matcher);
 }
 
+InterpreterAssemblerTest::InterpreterAssemblerForTest::
+    ~InterpreterAssemblerForTest() {
+  // Tests don't necessarily read and write accumulator but
+  // InterpreterAssembler checks accumulator uses.
+  if (Bytecodes::ReadsAccumulator(bytecode())) {
+    GetAccumulator();
+  }
+  if (Bytecodes::WritesAccumulator(bytecode())) {
+    SetAccumulator(nullptr);
+  }
+}
+
 Matcher<Node*> InterpreterAssemblerTest::InterpreterAssemblerForTest::IsLoad(
     const Matcher<LoadRepresentation>& rep_matcher,
     const Matcher<Node*>& base_matcher, const Matcher<Node*>& index_matcher) {
@@ -524,12 +536,16 @@ TARGET_TEST_F(InterpreterAssemblerTest, BytecodeOperand) {
 
 TARGET_TEST_F(InterpreterAssemblerTest, GetSetAccumulator) {
   TRACED_FOREACH(interpreter::Bytecode, bytecode, kBytecodes) {
+    if (!interpreter::Bytecodes::ReadsAccumulator(bytecode) ||
+        !interpreter::Bytecodes::WritesAccumulator(bytecode)) {
+      continue;
+    }
+
     InterpreterAssemblerForTest m(this, bytecode);
     // Should be incoming accumulator if not set.
     EXPECT_THAT(
         m.GetAccumulator(),
         IsParameter(InterpreterDispatchDescriptor::kAccumulatorParameter));
-
     // Should be set by SetAccumulator.
     Node* accumulator_value_1 = m.Int32Constant(0xdeadbeef);
     m.SetAccumulator(accumulator_value_1);

test/unittests/interpreter/interpreter-assembler-unittest.h

@@ -28,7 +28,7 @@ class InterpreterAssemblerTest : public TestWithIsolateAndZone {
         OperandScale operand_scale = OperandScale::kSingle)
         : InterpreterAssembler(test->isolate(), test->zone(), bytecode,
                                operand_scale) {}
-    ~InterpreterAssemblerForTest() override {}
+    ~InterpreterAssemblerForTest() override;
 
     Matcher<compiler::Node*> IsLoad(
         const Matcher<compiler::LoadRepresentation>& rep_matcher,