[turbofan] Make an OptionalOperator for MachineOperatorBuilder.

[turbofan] Make an OptionalOperator for MachineOperatorBuilder.

This makes usage of the MachineOperatorBuilder more robust, as it will be
an error to request an unsupported operator.

Along the way, I noticed that all 7 platforms support Float32Abs and
Float64Abs. Should make them non-optional in another CL?

R=bmeurer@chromium.org

Committed: https://crrev.com/deb5dcea5c6a23f5b1463fc02cb722a62a285c9b
Cr-Commit-Position: refs/heads/master@{#29223}

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

https://codereview.chromium.org/1128133003/diff/1/src/compiler/machine-operat...
File src/compiler/machine-operator.h (right):

https://codereview.chromium.org/1128133003/diff/1/src/compiler/machine-operat...
src/compiler/machine-operator.h:24: bool supported() const { return op_ !=
nullptr; }
Nit: Can we rename this to Supported() or IsSupported()?

https://codereview.chromium.org/1128133003/diff/1/test/unittests/compiler/mac...
File test/unittests/compiler/machine-operator-unittest.cc (right):

https://codereview.chromium.org/1128133003/diff/1/test/unittests/compiler/mac...
test/unittests/compiler/machine-operator-unittest.cc:162: }
Add // namespace comment after } and empty line before } helps to reach
agreement between cpplint and clang-format.

https://codereview.chromium.org/1128133003/diff/1/test/unittests/compiler/mac...
test/unittests/compiler/machine-operator-unittest.cc:170:
EXPECT_EQ((machine1.*constructor)(), (machine2.*constructor)());
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.

https://codereview.chromium.org/1128133003/diff/1/test/unittests/compiler/mac...
test/unittests/compiler/machine-operator-unittest.cc:208: PURE(Word32And, 2, 0,
1);
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:30]

https://codereview.chromium.org/1128133003/diff/1/src/compiler/machine-operat...
File src/compiler/machine-operator.h (right):

https://codereview.chromium.org/1128133003/diff/1/src/compiler/machine-operat...
src/compiler/machine-operator.h:24: bool supported() const { return op_ !=
nullptr; }
On 2015/05/07 04:48:38, Benedikt Meurer wrote:
> Nit: Can we rename this to Supported() or IsSupported()?

Done.

https://codereview.chromium.org/1128133003/diff/1/test/unittests/compiler/mac...
File test/unittests/compiler/machine-operator-unittest.cc (right):

https://codereview.chromium.org/1128133003/diff/1/test/unittests/compiler/mac...
test/unittests/compiler/machine-operator-unittest.cc:170:
EXPECT_EQ((machine1.*constructor)(), (machine2.*constructor)());
On 2015/05/07 04:48:38, Benedikt Meurer wrote:
> 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.

I can inline them directly into the MACRO.

https://codereview.chromium.org/1128133003/diff/1/test/unittests/compiler/mac...
test/unittests/compiler/machine-operator-unittest.cc:208: PURE(Word32And, 2, 0,
1);
On 2015/05/07 04:48:38, Benedikt Meurer wrote:
> 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.

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]

https://codereview.chromium.org/1128133003/diff/1/test/unittests/compiler/mac...
File test/unittests/compiler/machine-operator-unittest.cc (right):

https://codereview.chromium.org/1128133003/diff/1/test/unittests/compiler/mac...
test/unittests/compiler/machine-operator-unittest.cc:208: PURE(Word32And, 2, 0,
1);
On 2015/05/11 11:55:29, titzer wrote:
> On 2015/05/07 04:48:38, Benedikt Meurer wrote:
> > 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.
> 
> 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.

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?

[titzer, 2015-06-22 14:22:10]

On 2015/05/12 05:15:41, Benedikt Meurer wrote:
>
https://codereview.chromium.org/1128133003/diff/1/test/unittests/compiler/mac...
> File test/unittests/compiler/machine-operator-unittest.cc (right):
> 
>
https://codereview.chromium.org/1128133003/diff/1/test/unittests/compiler/mac...
> test/unittests/compiler/machine-operator-unittest.cc:208: PURE(Word32And, 2,
0,
> 1);
> On 2015/05/11 11:55:29, titzer wrote:
> > On 2015/05/07 04:48:38, Benedikt Meurer wrote:
> > > 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.
> > 
> > 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.
> 
> 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?

Done

[Benedikt Meurer, 2015-06-23 02:34:25]

Nice, thanks. LGTM

[titzer, 2015-06-23 08:17:52]

The CQ bit was checked by titzer@chromium.org

[commit-bot: I haz the power, 2015-06-23 08:17:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1128133003/60001

[commit-bot: I haz the power, 2015-06-23 08:24:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-23 08:24:57]

Try jobs failed on following builders:
  v8_win_nosnap_shared_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_compil...)

[titzer, 2015-06-23 10:33:58]

The CQ bit was checked by titzer@chromium.org

[titzer, 2015-06-23 10:33:58]

The patchset sent to the CQ was uploaded after l-g-t-m from bmeurer@chromium.org
Link to the patchset: https://codereview.chromium.org/1128133003/#ps80001
(title: "OPTIONAL macro")

[commit-bot: I haz the power, 2015-06-23 10:34:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1128133003/80001

[commit-bot: I haz the power, 2015-06-23 10:35:38]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-06-23 10:35:54]

Patchset 5 (id:??) landed as
https://crrev.com/deb5dcea5c6a23f5b1463fc02cb722a62a285c9b
Cr-Commit-Position: refs/heads/master@{#29223}