Initial import of ChangeLowering.

Initial import of ChangeLowering.

ChangeLowering is a Reducer to lower simplified change operators
to machine subgraphs. This initial version supports ChangeBitToBool,
ChangeBoolToBit, ChangeTaggedToFloat64 and ChangeInt32ToTagged.

TEST=compiler-unittests/change-lowering-unittest
BUG=v8:3489
LOG=n
R=jarin@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=23068

[Benedikt Meurer, 2014-08-12 06:30:52]

PTAL

[Jarin, 2014-08-12 08:22:38]

lgtm

https://codereview.chromium.org/460593003/diff/1/test/compiler-unittests/node...
File test/compiler-unittests/node-matchers.cc (right):

https://codereview.chromium.org/460593003/diff/1/test/compiler-unittests/node...
test/compiler-unittests/node-matchers.cc:54: MatchResultListener* listener)
const {
Should not this be V8_OVERRIDE?

[Benedikt Meurer, 2014-08-12 08:24:02]

https://codereview.chromium.org/460593003/diff/1/test/compiler-unittests/node...
File test/compiler-unittests/node-matchers.cc (right):

https://codereview.chromium.org/460593003/diff/1/test/compiler-unittests/node...
test/compiler-unittests/node-matchers.cc:54: MatchResultListener* listener)
const {
On 2014/08/12 08:22:38, jarin wrote:
> Should not this be V8_OVERRIDE?

Done.

[Benedikt Meurer, 2014-08-12 08:24:36]

Committed patchset #2 manually as 23068 (presubmit successful).

[titzer, 2014-08-13 08:59:47]

https://codereview.chromium.org/460593003/diff/20001/src/compiler/change-lowe...
File src/compiler/change-lowering.cc (right):

https://codereview.chromium.org/460593003/diff/20001/src/compiler/change-lowe...
src/compiler/change-lowering.cc:27: Node*
ChangeLoweringBase::ExternalConstant(ExternalReference reference) {
This is a big duplication of code. Why didn't you keep the original code with
used JSGraph instead?

https://codereview.chromium.org/460593003/diff/20001/src/compiler/change-lowe...
src/compiler/change-lowering.cc:138: Reduction
ChangeLowering<4>::ChangeBoolToBit(Node* val) {
This templatization of the changes is unnecessary complexity. The original code
had a simple branch on the pointer size.

https://codereview.chromium.org/460593003/diff/20001/src/compiler/change-lowe...
src/compiler/change-lowering.cc:200: Reduction
ChangeLowering<4>::ChangeTaggedToFloat64(Node* val, Node* effect,
ChangeLowering<4> and ChangeLowering<8> of this method are almost exactly alike.

[Benedikt Meurer, 2014-08-13 09:03:42]

https://codereview.chromium.org/460593003/diff/20001/src/compiler/change-lowe...
File src/compiler/change-lowering.cc (right):

https://codereview.chromium.org/460593003/diff/20001/src/compiler/change-lowe...
src/compiler/change-lowering.cc:138: Reduction
ChangeLowering<4>::ChangeBoolToBit(Node* val) {
Before it was simple and not unit testable because it depends on some global
state. Now it's simple and unit testable.

https://codereview.chromium.org/460593003/diff/20001/src/compiler/change-lowe...
src/compiler/change-lowering.cc:200: Reduction
ChangeLowering<4>::ChangeTaggedToFloat64(Node* val, Node* effect,
Yap, will be refactored once everythings in place.

[titzer, 2014-08-13 09:11:13]

https://codereview.chromium.org/460593003/diff/20001/src/compiler/change-lowe...
File src/compiler/change-lowering.cc (right):

https://codereview.chromium.org/460593003/diff/20001/src/compiler/change-lowe...
src/compiler/change-lowering.cc:138: Reduction
ChangeLowering<4>::ChangeBoolToBit(Node* val) {
On 2014/08/13 09:03:41, Benedikt Meurer wrote:
> Before it was simple and not unit testable because it depends on some global
> state. Now it's simple and unit testable.

You could, e.g. make the pointer size a field on the class, and then you don't
need to templatize it.

[Benedikt Meurer, 2014-08-13 09:14:13]

https://codereview.chromium.org/460593003/diff/20001/src/compiler/change-lowe...
File src/compiler/change-lowering.cc (right):

https://codereview.chromium.org/460593003/diff/20001/src/compiler/change-lowe...
src/compiler/change-lowering.cc:138: Reduction
ChangeLowering<4>::ChangeBoolToBit(Node* val) {
That's another option, sure. But I decided to do it this way.

[titzer, 2014-08-13 09:19:45]

On 2014/08/13 09:14:13, Benedikt Meurer wrote:
>
https://codereview.chromium.org/460593003/diff/20001/src/compiler/change-lowe...
> File src/compiler/change-lowering.cc (right):
> 
>
https://codereview.chromium.org/460593003/diff/20001/src/compiler/change-lowe...
> src/compiler/change-lowering.cc:138: Reduction
> ChangeLowering<4>::ChangeBoolToBit(Node* val) {
> That's another option, sure. But I decided to do it this way.

This is not necessary complexity. You undid a simple branch and replaced it with
template specialization on the pointer size. This an abuse of templates with
clear disadvantages and little or no benefit.

Can we please go back to the simpler code?

[Benedikt Meurer, 2014-08-13 09:26:41]

On 2014/08/13 at 09:19:45, titzer wrote:
> On 2014/08/13 09:14:13, Benedikt Meurer wrote:
> >
https://codereview.chromium.org/460593003/diff/20001/src/compiler/change-lowe...
> > File src/compiler/change-lowering.cc (right):
> > 
> >
https://codereview.chromium.org/460593003/diff/20001/src/compiler/change-lowe...
> > src/compiler/change-lowering.cc:138: Reduction
> > ChangeLowering<4>::ChangeBoolToBit(Node* val) {
> > That's another option, sure. But I decided to do it this way.
> 
> This is not necessary complexity. You undid a simple branch and replaced it
with template specialization on the pointer size. This an abuse of templates
with clear disadvantages and little or no benefit.
> 
> Can we please go back to the simpler code?

It was "a simple branch" because it was using the global state implicitly (i.e.
WordEqual and friends). Of course it's simple if you solve only a subset of the
actual problem. Doing it right will require the "simple branch" in many places,
which is probably less readable than just having specialized templates.

Note that this class is not finished yet, and because we can now actually test
it, it'll be easy to refactor and remove the duplication.

[titzer, 2014-08-13 09:41:02]

On 2014/08/13 09:26:41, Benedikt Meurer wrote:
> On 2014/08/13 at 09:19:45, titzer wrote:
> > On 2014/08/13 09:14:13, Benedikt Meurer wrote:
> > >
>
https://codereview.chromium.org/460593003/diff/20001/src/compiler/change-lowe...
> > > File src/compiler/change-lowering.cc (right):
> > > 
> > >
>
https://codereview.chromium.org/460593003/diff/20001/src/compiler/change-lowe...
> > > src/compiler/change-lowering.cc:138: Reduction
> > > ChangeLowering<4>::ChangeBoolToBit(Node* val) {
> > > That's another option, sure. But I decided to do it this way.
> > 
> > This is not necessary complexity. You undid a simple branch and replaced it
> with template specialization on the pointer size. This an abuse of templates
> with clear disadvantages and little or no benefit.
> > 
> > Can we please go back to the simpler code?
> 
> It was "a simple branch" because it was using the global state implicitly
(i.e.
> WordEqual and friends). Of course it's simple if you solve only a subset of
the
> actual problem. Doing it right will require the "simple branch" in many
places,
> which is probably less readable than just having specialized templates.
> 
> Note that this class is not finished yet, and because we can now actually test
> it, it'll be easy to refactor and remove the duplication.

As I mentioned, a simple field on the class would suffice to get rid of the
dependency on the kPointerSize. A template is still IMHO overkill.

[Benedikt Meurer, 2014-08-13 09:43:47]

On 2014/08/13 at 09:41:02, titzer wrote:
> On 2014/08/13 09:26:41, Benedikt Meurer wrote:
> > On 2014/08/13 at 09:19:45, titzer wrote:
> > > On 2014/08/13 09:14:13, Benedikt Meurer wrote:
> > > >
> >
https://codereview.chromium.org/460593003/diff/20001/src/compiler/change-lowe...
> > > > File src/compiler/change-lowering.cc (right):
> > > > 
> > > >
> >
https://codereview.chromium.org/460593003/diff/20001/src/compiler/change-lowe...
> > > > src/compiler/change-lowering.cc:138: Reduction
> > > > ChangeLowering<4>::ChangeBoolToBit(Node* val) {
> > > > That's another option, sure. But I decided to do it this way.
> > > 
> > > This is not necessary complexity. You undid a simple branch and replaced
it
> > with template specialization on the pointer size. This an abuse of templates
> > with clear disadvantages and little or no benefit.
> > > 
> > > Can we please go back to the simpler code?
> > 
> > It was "a simple branch" because it was using the global state implicitly
(i.e.
> > WordEqual and friends). Of course it's simple if you solve only a subset of
the
> > actual problem. Doing it right will require the "simple branch" in many
places,
> > which is probably less readable than just having specialized templates.
> > 
> > Note that this class is not finished yet, and because we can now actually
test
> > it, it'll be easy to refactor and remove the duplication.
> 
> As I mentioned, a simple field on the class would suffice to get rid of the
dependency on the kPointerSize. A template is still IMHO overkill.

As I mentioned, you'll still need the simple branch on the simple field. So
that's just a matter of "template specialization" vs. "if specialization".

Let's see how things work out and if the template version is really much more
complex compare to the if version, then we can easily refactor that.

[titzer, 2014-08-13 09:54:27]

On 2014/08/13 09:43:47, Benedikt Meurer wrote:
> On 2014/08/13 at 09:41:02, titzer wrote:
> > On 2014/08/13 09:26:41, Benedikt Meurer wrote:
> > > On 2014/08/13 at 09:19:45, titzer wrote:
> > > > On 2014/08/13 09:14:13, Benedikt Meurer wrote:
> > > > >
> > >
>
https://codereview.chromium.org/460593003/diff/20001/src/compiler/change-lowe...
> > > > > File src/compiler/change-lowering.cc (right):
> > > > > 
> > > > >
> > >
>
https://codereview.chromium.org/460593003/diff/20001/src/compiler/change-lowe...
> > > > > src/compiler/change-lowering.cc:138: Reduction
> > > > > ChangeLowering<4>::ChangeBoolToBit(Node* val) {
> > > > > That's another option, sure. But I decided to do it this way.
> > > > 
> > > > This is not necessary complexity. You undid a simple branch and replaced
> it
> > > with template specialization on the pointer size. This an abuse of
templates
> > > with clear disadvantages and little or no benefit.
> > > > 
> > > > Can we please go back to the simpler code?
> > > 
> > > It was "a simple branch" because it was using the global state implicitly
> (i.e.
> > > WordEqual and friends). Of course it's simple if you solve only a subset
of
> the
> > > actual problem. Doing it right will require the "simple branch" in many
> places,
> > > which is probably less readable than just having specialized templates.
> > > 
> > > Note that this class is not finished yet, and because we can now actually
> test
> > > it, it'll be easy to refactor and remove the duplication.
> > 
> > As I mentioned, a simple field on the class would suffice to get rid of the
> dependency on the kPointerSize. A template is still IMHO overkill.
> 
> As I mentioned, you'll still need the simple branch on the simple field. So
> that's just a matter of "template specialization" vs. "if specialization".
> 
> Let's see how things work out and if the template version is really much more
> complex compare to the if version, then we can easily refactor that.

Can we please do the simpler thing first? We shouldn't be using templates like
this until we are forced to really really good reasons.

Do the simpler thing first. It was already simple. Just go back to that.

BTW the MachineOperator builder already has a built-in word size, so you don't
even need a field on this class, just check the word size of the one passed in,
and in the test you can pass in one or the other.

[Benedikt Meurer, 2014-08-13 10:05:38]

On 2014/08/13 at 09:54:27, titzer wrote:
> On 2014/08/13 09:43:47, Benedikt Meurer wrote:
> > On 2014/08/13 at 09:41:02, titzer wrote:
> > > On 2014/08/13 09:26:41, Benedikt Meurer wrote:
> > > > On 2014/08/13 at 09:19:45, titzer wrote:
> > > > > On 2014/08/13 09:14:13, Benedikt Meurer wrote:
> > > > > >
> > > >
> >
https://codereview.chromium.org/460593003/diff/20001/src/compiler/change-lowe...
> > > > > > File src/compiler/change-lowering.cc (right):
> > > > > > 
> > > > > >
> > > >
> >
https://codereview.chromium.org/460593003/diff/20001/src/compiler/change-lowe...
> > > > > > src/compiler/change-lowering.cc:138: Reduction
> > > > > > ChangeLowering<4>::ChangeBoolToBit(Node* val) {
> > > > > > That's another option, sure. But I decided to do it this way.
> > > > > 
> > > > > This is not necessary complexity. You undid a simple branch and
replaced
> > it
> > > > with template specialization on the pointer size. This an abuse of
templates
> > > > with clear disadvantages and little or no benefit.
> > > > > 
> > > > > Can we please go back to the simpler code?
> > > > 
> > > > It was "a simple branch" because it was using the global state
implicitly
> > (i.e.
> > > > WordEqual and friends). Of course it's simple if you solve only a subset
of
> > the
> > > > actual problem. Doing it right will require the "simple branch" in many
> > places,
> > > > which is probably less readable than just having specialized templates.
> > > > 
> > > > Note that this class is not finished yet, and because we can now
actually
> > test
> > > > it, it'll be easy to refactor and remove the duplication.
> > > 
> > > As I mentioned, a simple field on the class would suffice to get rid of
the
> > dependency on the kPointerSize. A template is still IMHO overkill.
> > 
> > As I mentioned, you'll still need the simple branch on the simple field. So
> > that's just a matter of "template specialization" vs. "if specialization".
> > 
> > Let's see how things work out and if the template version is really much
more
> > complex compare to the if version, then we can easily refactor that.
> 
> Can we please do the simpler thing first? We shouldn't be using templates like
this until we are forced to really really good reasons.
> 
> Do the simpler thing first. It was already simple. Just go back to that.
> 
> BTW the MachineOperator builder already has a built-in word size, so you don't
even need a field on this class, just check the word size of the one passed in,
and in the test you can pass in one or the other.

As this is just a matter of personal preference, I'll do the actual work first
and check if it makes sense based on the result.