Refactor Instruction classes to use a template base class.

Refactor Instruction classes to use a template base class.

This change makes instructions and computations more uniform.

Also make CreateArrayComp take a register input operand. This
is done by changing the convenience DECLARE_COMPUTATION macro accordingly and use
TemplateComputation as base class for call-instructions.
Committed: https://code.google.com/p/dart/source/detail?r=10330

[Florian Schneider, 2012-08-06 15:37:31]

Adding srdjan@.

To check if templates have any negative effect on binary size, I checked the
resulting dart binary and it looks ok: It is actually 4Kbytes smaller than
before.

[Kevin Millikin (Google), 2012-08-07 10:06:03]

LGTM.

http://codereview.chromium.org/10831178/diff/1/vm/intermediate_language_ia32.cc
File vm/intermediate_language_ia32.cc (right):

http://codereview.chromium.org/10831178/diff/1/vm/intermediate_language_ia32....
vm/intermediate_language_ia32.cc:1026: const intptr_t kNumInputs = 1;
I have a feeling that this is always InputCount() for all
instructions/computations.  I think it would be clearer to write:

new LocationSummary(InputCount(), kNumTemps, LocationSummary::kCall);

Let's consider that for a future change.

http://codereview.chromium.org/10831178/diff/1/vm/intermediate_language_ia32....
vm/intermediate_language_ia32.cc:1030: locs->set_in(0,
Location::RegisterLocation(ECX));
I don't really like the number '0' here.  It has to agree in a bunch of places
(the initializer in the constructor, the get accessor, here, and in the codegen
when it's used).  It might be easier to have a "layout" enum describing the
inputs:

enum Layout {
  kElementType,
};

...

locs->set_in(kElementType, Location::RegisterLocation(ECX));

========

Another option is to make Value into a pair of the SSA value and the location
and Definition contain the output location, rather than splitting
(non-temporary) locations out into a separate location summary.  I suppose we
still need a platform-specific list of indexed temporary locations, but
otherwise it seems cleaner.

In class Value:

Location& location() { return location_; }

In class CreateArrayComp:

Value* element_type() const { return inputs_[0]; }

...

// and here:

element_type().location() = Location::RegisterLocation(ECX);

http://codereview.chromium.org/10831178/diff/1/vm/intermediate_language_ia32....
vm/intermediate_language_ia32.cc:1038: ASSERT(locs()->in(0).reg() == ECX);
There's the 0 again.

[Florian Schneider, 2012-08-07 10:23:55]

http://codereview.chromium.org/10831178/diff/1/vm/intermediate_language_ia32.cc
File vm/intermediate_language_ia32.cc (right):

http://codereview.chromium.org/10831178/diff/1/vm/intermediate_language_ia32....
vm/intermediate_language_ia32.cc:1026: const intptr_t kNumInputs = 1;
On 2012/08/07 10:06:03, kmillikin wrote:
> I have a feeling that this is always InputCount() for all
> instructions/computations.  I think it would be clearer to write:
> 
> new LocationSummary(InputCount(), kNumTemps, LocationSummary::kCall);
> 
> Let's consider that for a future change.

Yes, that will be the case once all call-instructions report the correct input
count via InputCount(). There are few instructions left, that don't right now,
but I'll fix those in my next couple of CLs.

http://codereview.chromium.org/10831178/diff/1/vm/intermediate_language_ia32....
vm/intermediate_language_ia32.cc:1030: locs->set_in(0,
Location::RegisterLocation(ECX));
On 2012/08/07 10:06:03, kmillikin wrote:
> I don't really like the number '0' here.  It has to agree in a bunch of places
> (the initializer in the constructor, the get accessor, here, and in the
codegen
> when it's used).  It might be easier to have a "layout" enum describing the
> inputs:
> 
> enum Layout {
>   kElementType,
> };
> 
> ...
> 
> locs->set_in(kElementType, Location::RegisterLocation(ECX));
> 
> ========
> 
> Another option is to make Value into a pair of the SSA value and the location
> and Definition contain the output location, rather than splitting
> (non-temporary) locations out into a separate location summary.  I suppose we
> still need a platform-specific list of indexed temporary locations, but
> otherwise it seems cleaner.
> 
> In class Value:
> 
> Location& location() { return location_; }
> 
> In class CreateArrayComp:
> 
> Value* element_type() const { return inputs_[0]; }
> 
> ...
> 
> // and here:
> 
> element_type().location() = Location::RegisterLocation(ECX);

Agreed. Let's consider removing those 0s and 1s in a future CL.