[turbofan] Make an OptionalOperator for MachineOperatorBuilder.

test/unittests/compiler/machine-operator-unittest.cc

Index: test/unittests/compiler/machine-operator-unittest.cc
diff --git a/test/unittests/compiler/machine-operator-unittest.cc b/test/unittests/compiler/machine-operator-unittest.cc
index 31f55793c3f881f8c6d4c4ffc473a4431cf2def5..02bf3e8fe5fb1879005aa5210d363d807407812c 100644
--- a/test/unittests/compiler/machine-operator-unittest.cc
+++ b/test/unittests/compiler/machine-operator-unittest.cc
@@ -149,132 +149,159 @@ INSTANTIATE_TEST_CASE_P(
         ::testing::Combine(::testing::ValuesIn(kMachineTypes),
                            ::testing::Values(kNoWriteBarrier,
                                              kFullWriteBarrier))));
-
+#endif
 
 // -----------------------------------------------------------------------------
 // Pure operators.
 
-
 namespace {
 
-struct PureOperator {
-  const Operator* (MachineOperatorBuilder::*constructor)();
-  IrOpcode::Value opcode;
-  int value_input_count;
-  int control_input_count;
-  int value_output_count;
-};
-
-
-std::ostream& operator<<(std::ostream& os, const PureOperator& pop) {
-  return os << IrOpcode::Mnemonic(pop.opcode);
+typedef const Operator* (MachineOperatorBuilder::*OperatorConstructor)();
+typedef const OptionalOperator (
+    MachineOperatorBuilder::*OptionalOperatorConstructor)();
 }

[Benedikt Meurer, 2015/05/07 04:48:38]

Add // namespace comment after } and empty line before } helps to reach
agreement between cpplint and clang-format.

 
 
-const PureOperator kPureOperators[] = {
-#define PURE(Name, value_input_count, control_input_count, value_output_count) \
-  {                                                                            \
-    &MachineOperatorBuilder::Name, IrOpcode::k##Name, value_input_count,       \
-        control_input_count, value_output_count                                \
+class MachinePureOperatorTest : public TestWithZone {
+ protected:
+  void CheckGlobalCaching(OperatorConstructor constructor) {
+    MachineOperatorBuilder machine1(zone(), word_type());
+    MachineOperatorBuilder machine2(zone(), word_type());
+    EXPECT_EQ((machine1.*constructor)(), (machine2.*constructor)());

[Benedikt Meurer, 2015/05/07 04:48:38]

Don't move test expectations into helper functions unless you absolutely have
to. That way defeat the benefits of unit testing. The EXPECT_* and ASSERT_*
statements should always be in the toplevel test function itself, so you know
immediately what went wrong and why.

[titzer, 2015/05/11 11:55:29]

I can inline them directly into the MACRO.

   }
-    PURE(Word32And, 2, 0, 1), PURE(Word32Or, 2, 0, 1), PURE(Word32Xor, 2, 0, 1),
-    PURE(Word32Shl, 2, 0, 1), PURE(Word32Shr, 2, 0, 1),
-    PURE(Word32Sar, 2, 0, 1), PURE(Word32Ror, 2, 0, 1),
-    PURE(Word32Equal, 2, 0, 1), PURE(Word32Clz, 1, 0, 1),
-    PURE(Word64And, 2, 0, 1), PURE(Word64Or, 2, 0, 1), PURE(Word64Xor, 2, 0, 1),
-    PURE(Word64Shl, 2, 0, 1), PURE(Word64Shr, 2, 0, 1),
-    PURE(Word64Sar, 2, 0, 1), PURE(Word64Ror, 2, 0, 1),
-    PURE(Word64Equal, 2, 0, 1), PURE(Int32Add, 2, 0, 1),
-    PURE(Int32AddWithOverflow, 2, 0, 2), PURE(Int32Sub, 2, 0, 1),
-    PURE(Int32SubWithOverflow, 2, 0, 2), PURE(Int32Mul, 2, 0, 1),
-    PURE(Int32MulHigh, 2, 0, 1), PURE(Int32Div, 2, 1, 1),
-    PURE(Uint32Div, 2, 1, 1), PURE(Int32Mod, 2, 1, 1), PURE(Uint32Mod, 2, 1, 1),
-    PURE(Int32LessThan, 2, 0, 1), PURE(Int32LessThanOrEqual, 2, 0, 1),
-    PURE(Uint32LessThan, 2, 0, 1), PURE(Uint32LessThanOrEqual, 2, 0, 1),
-    PURE(Int64Add, 2, 0, 1), PURE(Int64Sub, 2, 0, 1), PURE(Int64Mul, 2, 0, 1),
-    PURE(Int64Div, 2, 0, 1), PURE(Uint64Div, 2, 0, 1), PURE(Int64Mod, 2, 0, 1),
-    PURE(Uint64Mod, 2, 0, 1), PURE(Int64LessThan, 2, 0, 1),
-    PURE(Int64LessThanOrEqual, 2, 0, 1), PURE(Uint64LessThan, 2, 0, 1),
-    PURE(ChangeFloat32ToFloat64, 1, 0, 1), PURE(ChangeFloat64ToInt32, 1, 0, 1),
-    PURE(ChangeFloat64ToUint32, 1, 0, 1), PURE(ChangeInt32ToInt64, 1, 0, 1),
-    PURE(ChangeUint32ToFloat64, 1, 0, 1), PURE(ChangeUint32ToUint64, 1, 0, 1),
-    PURE(TruncateFloat64ToFloat32, 1, 0, 1),
-    PURE(TruncateFloat64ToInt32, 1, 0, 1), PURE(TruncateInt64ToInt32, 1, 0, 1),
-    PURE(Float32Add, 2, 0, 1), PURE(Float32Sub, 2, 0, 1),
-    PURE(Float32Mul, 2, 0, 1), PURE(Float32Div, 2, 0, 1),
-    PURE(Float32Abs, 1, 0, 1), PURE(Float32Sqrt, 1, 0, 1),
-    PURE(Float32Equal, 2, 0, 1), PURE(Float32LessThan, 2, 0, 1),
-    PURE(Float32LessThanOrEqual, 2, 0, 1), PURE(Float32Max, 2, 0, 1),
-    PURE(Float32Min, 2, 0, 1), PURE(Float64Add, 2, 0, 1),
-    PURE(Float64Sub, 2, 0, 1), PURE(Float64Mul, 2, 0, 1),
-    PURE(Float64Div, 2, 0, 1), PURE(Float64Mod, 2, 0, 1),
-    PURE(Float64Abs, 1, 0, 1), PURE(Float64Sqrt, 1, 0, 1),
-    PURE(Float64Equal, 2, 0, 1), PURE(Float64LessThan, 2, 0, 1),
-    PURE(Float64LessThanOrEqual, 2, 0, 1), PURE(Float64Max, 2, 0, 1),
-    PURE(Float64Min, 2, 0, 1), PURE(LoadStackPointer, 0, 0, 1),
-    PURE(Float64RoundDown, 1, 0, 1), PURE(Float64RoundTruncate, 1, 0, 1),
-    PURE(Float64RoundTiesAway, 1, 0, 1), PURE(Float64ExtractLowWord32, 1, 0, 1),
-    PURE(Float64ExtractHighWord32, 1, 0, 1),
-    PURE(Float64InsertLowWord32, 2, 0, 1),
-    PURE(Float64InsertHighWord32, 2, 0, 1)
-#undef PURE
-};
-
-
-typedef MachineOperatorTestWithParam<PureOperator> MachinePureOperatorTest;
-
-}  // namespace
-
-
-TEST_P(MachinePureOperatorTest, InstancesAreGloballyShared) {
-  const PureOperator& pop = GetParam();
-  MachineOperatorBuilder machine1(zone(), type());
-  MachineOperatorBuilder machine2(zone(), type());
-  EXPECT_EQ((machine1.*pop.constructor)(), (machine2.*pop.constructor)());
-}
 
+  void CheckCounts(OperatorConstructor constructor, int value_input_count,
+                   int control_input_count, int value_output_count) {
+    MachineOperatorBuilder machine(zone(), word_type());
+    const Operator* op = (machine.*constructor)();
+    EXPECT_EQ(value_input_count, op->ValueInputCount());
+    EXPECT_EQ(control_input_count, op->ControlInputCount());
+    EXPECT_EQ(value_output_count, op->ValueOutputCount());
+  }
 
-TEST_P(MachinePureOperatorTest, NumberOfInputsAndOutputs) {
-  MachineOperatorBuilder machine(zone(), type());
-  const PureOperator& pop = GetParam();
-  const Operator* op = (machine.*pop.constructor)();
+  void CheckGlobalCaching(OptionalOperatorConstructor constructor,
+                          MachineOperatorBuilder::Flag enabling_flag) {
+    MachineOperatorBuilder machine1(zone(), word_type(), enabling_flag);
+    MachineOperatorBuilder machine2(zone(), word_type(), enabling_flag);
+    EXPECT_EQ((machine1.*constructor)().op(), (machine2.*constructor)().op());
+  }
 
-  EXPECT_EQ(pop.value_input_count, op->ValueInputCount());
-  EXPECT_EQ(0, op->EffectInputCount());
-  EXPECT_EQ(pop.control_input_count, op->ControlInputCount());
-  EXPECT_EQ(pop.value_input_count + pop.control_input_count,
-            OperatorProperties::GetTotalInputCount(op));
+  void CheckEnablingFlag(OptionalOperatorConstructor constructor,
+                         MachineOperatorBuilder::Flag enabling_flag) {
+    MachineOperatorBuilder machine1(zone(), word_type(), enabling_flag);
+    MachineOperatorBuilder machine2(zone(), word_type());
+    EXPECT_TRUE((machine1.*constructor)().supported());
+    EXPECT_FALSE((machine2.*constructor)().supported());
+  }
 
-  EXPECT_EQ(pop.value_output_count, op->ValueOutputCount());
-  EXPECT_EQ(0, op->EffectOutputCount());
-  EXPECT_EQ(0, op->ControlOutputCount());
-}
+ private:
+  MachineType word_type() { return kMachPtr; }
+};
 
 
-TEST_P(MachinePureOperatorTest, MarkedAsPure) {
-  MachineOperatorBuilder machine(zone(), type());
-  const PureOperator& pop = GetParam();
-  const Operator* op = (machine.*pop.constructor)();
-  EXPECT_TRUE(op->HasProperty(Operator::kPure));
+TEST_F(MachinePureOperatorTest, PureOperators) {
+#define PURE(Name, value_input_count, control_input_count, value_output_count) \
+  CheckGlobalCaching(&MachineOperatorBuilder::Name);                           \
+  CheckCounts(&MachineOperatorBuilder::Name, value_input_count,                \
+              control_input_count, value_output_count);
+
+  PURE(Word32And, 2, 0, 1);

[Benedikt Meurer, 2015/05/07 04:48:38]

This is a lot of boilerplate and macroism and indirection for no obvious
benefit. Why didn't you implement the loop that we talked about yesterday? I.e.
something simple like this:

namespace {

struct PureOperator {
  const Operator* (MachineOperatorBuilder::*constructor)();
  char const* const constructor_name;
  int value_input_count;
  int control_input_count;
  int value_output_count;
};


std::ostream& operator<<(std::ostream& os, PureOperator  const& pop) {
  return os << pop.constructor_name;
}

const PureOperator kPureOperators[] = {
#define PURE(Name, value_input_count, control_input_count, value_output_count)
{&MachineOperatorBuilder::Name, #Name,value_input_count, control_input_count,
value_output_count}
  PURE(Word32Or, 2, 0, 1),
  ...
#undef PURE
};

}  // namespace


TEST_F(MachineOperatorTest, PureOperators) {
  TRACED_FOREACH(MachineType, machine_rep1, kMachineReps) {
    MachineOperatorBuilder machine1(zone(), machine_rep1);
    TRACED_FOREACH(MachineType, machine_rep2, kMachineReps) {
      MachineOperatorBuilder machine2(zone(), machine_rep2);
      TRACED_FOREACH(PureOperator, pop, kPureOperators) {
        const Operator* op1 = (machine1.pop.constructor)();
        const Operator* op2 = (machine2.pop.constructor)();
        EXPECT_EQ(op1, op2);
        EXPECT_EQ(pop.value_input_count, op1->ValueInputCount());
        EXPECT_EQ(pop.control_input_count, op1->ControlInputCount());
        EXPECT_EQ(pop.value_output_count, op1->ValueOutputCount());
      }
    }
  }
}

Same for the optional ones. Move it to the last section in the file,
MachineOperatorTest is already there.

[titzer, 2015/05/11 11:55:29]

I can inline the helper functions into the macro, but I don't see how the
TRACED_FOR_EACH with a global array of data would be better than just
enumerating them directly in the test function.

[Benedikt Meurer, 2015/05/12 05:15:41]

What's the point of putting everything into the macro if you can solve it in an
elegant way instead that involves only a trivial macro to initialize array
elements?

+  PURE(Word32Or, 2, 0, 1);
+  PURE(Word32Xor, 2, 0, 1);
+  PURE(Word32Shl, 2, 0, 1);
+  PURE(Word32Shr, 2, 0, 1);
+  PURE(Word32Sar, 2, 0, 1);
+  PURE(Word32Ror, 2, 0, 1);
+  PURE(Word32Equal, 2, 0, 1);
+  PURE(Word32Clz, 1, 0, 1);
+  PURE(Word64And, 2, 0, 1);
+  PURE(Word64Or, 2, 0, 1);
+  PURE(Word64Xor, 2, 0, 1);
+  PURE(Word64Shl, 2, 0, 1);
+  PURE(Word64Shr, 2, 0, 1);
+  PURE(Word64Sar, 2, 0, 1);
+  PURE(Word64Ror, 2, 0, 1);
+  PURE(Word64Equal, 2, 0, 1);
+  PURE(Int32Add, 2, 0, 1);
+  PURE(Int32AddWithOverflow, 2, 0, 2);
+  PURE(Int32Sub, 2, 0, 1);
+  PURE(Int32SubWithOverflow, 2, 0, 2);
+  PURE(Int32Mul, 2, 0, 1);
+  PURE(Int32MulHigh, 2, 0, 1);
+  PURE(Int32Div, 2, 1, 1);
+  PURE(Uint32Div, 2, 1, 1);
+  PURE(Int32Mod, 2, 1, 1);
+  PURE(Uint32Mod, 2, 1, 1);
+  PURE(Int32LessThan, 2, 0, 1);
+  PURE(Int32LessThanOrEqual, 2, 0, 1);
+  PURE(Uint32LessThan, 2, 0, 1);
+  PURE(Uint32LessThanOrEqual, 2, 0, 1);
+  PURE(Int64Add, 2, 0, 1);
+  PURE(Int64Sub, 2, 0, 1);
+  PURE(Int64Mul, 2, 0, 1);
+  PURE(Int64Div, 2, 0, 1);
+  PURE(Uint64Div, 2, 0, 1);
+  PURE(Int64Mod, 2, 0, 1);
+  PURE(Uint64Mod, 2, 0, 1);
+  PURE(Int64LessThan, 2, 0, 1);
+  PURE(Int64LessThanOrEqual, 2, 0, 1);
+  PURE(Uint64LessThan, 2, 0, 1);
+  PURE(ChangeFloat32ToFloat64, 1, 0, 1);
+  PURE(ChangeFloat64ToInt32, 1, 0, 1);
+  PURE(ChangeFloat64ToUint32, 1, 0, 1);
+  PURE(ChangeInt32ToInt64, 1, 0, 1);
+  PURE(ChangeUint32ToFloat64, 1, 0, 1);
+  PURE(ChangeUint32ToUint64, 1, 0, 1);
+  PURE(TruncateFloat64ToFloat32, 1, 0, 1);
+  PURE(TruncateFloat64ToInt32, 1, 0, 1);
+  PURE(TruncateInt64ToInt32, 1, 0, 1);
+  PURE(Float32Add, 2, 0, 1);
+  PURE(Float32Sub, 2, 0, 1);
+  PURE(Float32Mul, 2, 0, 1);
+  PURE(Float32Div, 2, 0, 1);
+  PURE(Float32Sqrt, 1, 0, 1);
+  PURE(Float32Equal, 2, 0, 1);
+  PURE(Float32LessThan, 2, 0, 1);
+  PURE(Float32LessThanOrEqual, 2, 0, 1);
+  PURE(Float64Add, 2, 0, 1);
+  PURE(Float64Sub, 2, 0, 1);
+  PURE(Float64Mul, 2, 0, 1);
+  PURE(Float64Div, 2, 0, 1);
+  PURE(Float64Mod, 2, 0, 1);
+  PURE(Float64Sqrt, 1, 0, 1);
+  PURE(Float64Equal, 2, 0, 1);
+  PURE(Float64LessThan, 2, 0, 1);
+  PURE(Float64LessThanOrEqual, 2, 0, 1);
+  PURE(LoadStackPointer, 0, 0, 1);
+  PURE(Float64ExtractLowWord32, 1, 0, 1);
+  PURE(Float64ExtractHighWord32, 1, 0, 1);
+  PURE(Float64InsertLowWord32, 2, 0, 1);
+  PURE(Float64InsertHighWord32, 2, 0, 1);
+#undef PURE
 }
 
 
-TEST_P(MachinePureOperatorTest, OpcodeIsCorrect) {
-  MachineOperatorBuilder machine(zone(), type());
-  const PureOperator& pop = GetParam();
-  const Operator* op = (machine.*pop.constructor)();
-  EXPECT_EQ(pop.opcode, op->opcode());
+TEST_F(MachinePureOperatorTest, PureOptionalOperators) {
+#define OPTIONAL(Name, value_input_count, control_input_count, \
+                 value_output_count)                           \
+  CheckGlobalCaching(&MachineOperatorBuilder::Name,            \
+                     MachineOperatorBuilder::k##Name);         \
+  CheckEnablingFlag(&MachineOperatorBuilder::Name,             \
+                    MachineOperatorBuilder::k##Name);
+
+  OPTIONAL(Float32Max, 2, 0, 1);
+  OPTIONAL(Float32Min, 2, 0, 1);
+  OPTIONAL(Float32Abs, 1, 0, 1);
+  OPTIONAL(Float64Abs, 1, 0, 1);
+  OPTIONAL(Float64Max, 2, 0, 1);
+  OPTIONAL(Float64Min, 2, 0, 1);
+  OPTIONAL(Float64RoundDown, 1, 0, 1);
+  OPTIONAL(Float64RoundTruncate, 1, 0, 1);
+  OPTIONAL(Float64RoundTiesAway, 1, 0, 1);
+#undef OPTIONAL
 }
 
 
-INSTANTIATE_TEST_CASE_P(
-    MachineOperatorTest, MachinePureOperatorTest,
-    ::testing::Combine(::testing::ValuesIn(kMachineReps),
-                       ::testing::ValuesIn(kPureOperators)));
-
-#endif  // GTEST_HAS_COMBINE
-
-
 // -----------------------------------------------------------------------------
 // Pseudo operators.