[Turbofan]: Use new MachineTypes in access-builder.

[Turbofan]: Use new MachineTypes in access-builder.

Introduced MachineType::TaggedSigned() and TaggedPointer().

The idea is to quit using the representational dimension of Type, and
instead encode this information in the MachineRepresentation (itself
lightly wrapped in MachineType, along with MachineSemantic).

There are three parts to the whole change:

1) Places that set the machine representation - constant nodes, loads nad
   stores, global object and native context specialization.

2) Places that propagate type/representation - this is representation
   inference (aka simplified lowering). At the end of this process we
   expect to have a MachineRepresentation for every node. An interesting
   part of this is phi merging.

3) Places that examine representation - WriteBarrier elimination does this.
   Currently it's looking at the Type representation dimension, but as
   a part of this change (or in a soon-to-follow change) it can simply
   examine the MachineRepresentation.

BUG=

Committed: https://crrev.com/56429fc14671a10749190a4dfeacd38b7270f6f5
Cr-Commit-Position: refs/heads/master@{#38978}

[mvstanton, 2016-08-22 12:17:50]

The CQ bit was checked by mvstanton@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-22 12:18:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mvstanton, 2016-08-22 12:36:28]

mvstanton@chromium.org changed reviewers:
+ jarin@chromium.org, mstarzinger@chromium.org

[mvstanton, 2016-08-22 12:36:28]

mvstanton@chromium.org changed required reviewers:
+ jarin@chromium.org

[mvstanton, 2016-08-22 12:36:29]

Hi Jaro,
Here is the second part of the work to use MachineTypes and get away from the
Type representation dimension. In this CL, we begin using the new TaggedPointer
and TaggedSigned in access-builder and a few other places.

New rules are added in representation-change.cc.

Thanks for the look!
--Mike

[commit-bot: I haz the power, 2016-08-22 12:40:19]

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

[commit-bot: I haz the power, 2016-08-22 12:40:20]

Dry run: Try jobs failed on following builders:
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/7365)
  v8_mac_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng_triggered/bui...)

[Michael Starzinger, 2016-08-22 12:59:46]

https://codereview.chromium.org/2258073002/diff/40001/src/compiler/js-generic...
File src/compiler/js-generic-lowering.cc (right):

https://codereview.chromium.org/2258073002/diff/40001/src/compiler/js-generic...
src/compiler/js-generic-lowering.cc:364: NodeProperties::ChangeOp(node,
machine()->Load(MachineType::TaggedPointer()));
This is the load of the actual context slot (not the context chain), this could
be any tagged value (including a Smi). IIUC then this should remain AnyTagged.

https://codereview.chromium.org/2258073002/diff/40001/src/compiler/js-generic...
src/compiler/js-generic-lowering.cc:603: NodeProperties::ChangeOp(node,
machine()->Load(MachineType::TaggedPointer()));
nit: The ThreadLocalTop::pending_message_obj_ is of type "Object*". Even though
it seems we are only storing tagged values there (either TheHole or a
JSMessageObject) I would still vote for keeping this just AnyTagged for now. Not
worth the risk optimizing this in any way IMHO. Also not that the JSStoreMessage
case below also uses kTagged.

https://codereview.chromium.org/2258073002/diff/40001/src/compiler/simplified...
File src/compiler/simplified-operator-reducer.cc (right):

https://codereview.chromium.org/2258073002/diff/40001/src/compiler/simplified...
src/compiler/simplified-operator-reducer.cc:71: Int32Matcher
m(node->InputAt(0));
nit: Seems like a copy of the below, why the expansion?

[Jarin, 2016-08-22 14:23:23]

Some comments below.

As discussed offline, it should be easier not to use the new representations in
generic lowering just yet - it will just create unnecessary friction.

A good first step would be using the new representation wherever we are using
the representation axis of Type.

https://codereview.chromium.org/2258073002/diff/40001/src/compiler/code-gener...
File src/compiler/code-generator.cc (right):

https://codereview.chromium.org/2258073002/diff/40001/src/compiler/code-gener...
src/compiler/code-generator.cc:880: type.representation() ==
MachineRepresentation::kTaggedPointer ||
Why has the tagged pointer appeared here?

https://codereview.chromium.org/2258073002/diff/40001/src/compiler/representa...
File src/compiler/representation-change.cc (right):

https://codereview.chromium.org/2258073002/diff/40001/src/compiler/representa...
src/compiler/representation-change.cc:196: break;
No need for break.

https://codereview.chromium.org/2258073002/diff/40001/src/compiler/representa...
src/compiler/representation-change.cc:199: // do it.
Remove comment.

https://codereview.chromium.org/2258073002/diff/40001/src/compiler/representa...
src/compiler/representation-change.cc:203: if (machine()->Is64() &&
output_type->Is(Type::Signed32())) {
Do you ever get here? If this is a 64-bit platform, then Signed32 = SignedSmall,
so this case would be handled above.

https://codereview.chromium.org/2258073002/diff/40001/src/compiler/representa...
src/compiler/representation-change.cc:233: } else if (output_rep ==
MachineRepresentation::kBit) {
This case is not necessary (since the final else goes to TypeError).

https://codereview.chromium.org/2258073002/diff/40001/src/compiler/representa...
src/compiler/representation-change.cc:242: // TaggedSigned? I think so because
we are on a 64bit platform.
Yes, on 64-but platform this produces a Smi.

https://codereview.chromium.org/2258073002/diff/40001/src/compiler/representa...
src/compiler/representation-change.cc:261: return node;  // No change necessary.
Actually, NumberConstant would be wrong because it could produce Smi.

I think for NumberConstants you should fall through.

https://codereview.chromium.org/2258073002/diff/40001/src/compiler/representa...
src/compiler/representation-change.cc:281: if
(output_type->Is(Type::SignedSmall())) {
Here you have to ask if it can be a Smi:

if (output_type->Maybe(Type::SignedSmall())) {

https://codereview.chromium.org/2258073002/diff/40001/src/compiler/representa...
src/compiler/representation-change.cc:622: if
(output_type->Is(Type::TaggedSigned())) {
You should not need this case. Maybe you want to add a TODO reminder here.

https://codereview.chromium.org/2258073002/diff/40001/src/compiler/representa...
src/compiler/representation-change.cc:627: op =
simplified()->ChangeTaggedToInt32();
This should be ChangeTaggedSignedToInt32, no?

https://codereview.chromium.org/2258073002/diff/40001/src/compiler/representa...
src/compiler/representation-change.cc:644: if
(output_type->Is(Type::TaggedSigned())) {
This case should go away. Maybe a TODO here?

[mvstanton, 2016-08-24 13:10:59]

The CQ bit was checked by mvstanton@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-24 13:11:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-24 13:14:19]

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

[commit-bot: I haz the power, 2016-08-24 13:14:21]

Dry run: Try jobs failed on following builders:
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/22475)

[mvstanton, 2016-08-24 13:27:17]

The CQ bit was checked by mvstanton@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-24 13:27:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-24 13:52:21]

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

[commit-bot: I haz the power, 2016-08-24 13:52:22]

Dry run: This issue passed the CQ dry run.

[mvstanton, 2016-08-24 16:04:53]

Patchset #4 (id:60001) has been deleted

[mvstanton, 2016-08-24 16:05:15]

The CQ bit was checked by mvstanton@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-24 16:05:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mvstanton, 2016-08-24 16:06:28]

Hi guys,
Thanks for the fantastic comments.
I finally addressed a write barrier problem, where TaggedPointer wasn't
recognized as it should be (like Tagged).

I believe I'll have to rebase before landing to remove ImpossibleToTagged - I'll
follow the conventions you guys outlined today.
Thanks,
--Mike

https://codereview.chromium.org/2258073002/diff/40001/src/compiler/code-gener...
File src/compiler/code-generator.cc (right):

https://codereview.chromium.org/2258073002/diff/40001/src/compiler/code-gener...
src/compiler/code-generator.cc:880: type.representation() ==
MachineRepresentation::kTaggedPointer ||
On 2016/08/22 14:23:22, Jarin wrote:
> Why has the tagged pointer appeared here?

There is no good reason. Ay dios mio. Removed.

https://codereview.chromium.org/2258073002/diff/40001/src/compiler/js-generic...
File src/compiler/js-generic-lowering.cc (right):

https://codereview.chromium.org/2258073002/diff/40001/src/compiler/js-generic...
src/compiler/js-generic-lowering.cc:364: NodeProperties::ChangeOp(node,
machine()->Load(MachineType::TaggedPointer()));
On 2016/08/22 12:59:46, Michael Starzinger wrote:
> This is the load of the actual context slot (not the context chain), this
could
> be any tagged value (including a Smi). IIUC then this should remain AnyTagged.

Done.

https://codereview.chromium.org/2258073002/diff/40001/src/compiler/js-generic...
src/compiler/js-generic-lowering.cc:603: NodeProperties::ChangeOp(node,
machine()->Load(MachineType::TaggedPointer()));
On 2016/08/22 12:59:46, Michael Starzinger wrote:
> nit: The ThreadLocalTop::pending_message_obj_ is of type "Object*". Even
though
> it seems we are only storing tagged values there (either TheHole or a
> JSMessageObject) I would still vote for keeping this just AnyTagged for now.
Not
> worth the risk optimizing this in any way IMHO. Also not that the
JSStoreMessage
> case below also uses kTagged.

Done.

https://codereview.chromium.org/2258073002/diff/40001/src/compiler/representa...
File src/compiler/representation-change.cc (right):

https://codereview.chromium.org/2258073002/diff/40001/src/compiler/representa...
src/compiler/representation-change.cc:196: break;
On 2016/08/22 14:23:22, Jarin wrote:
> No need for break.

Done.

https://codereview.chromium.org/2258073002/diff/40001/src/compiler/representa...
src/compiler/representation-change.cc:199: // do it.
On 2016/08/22 14:23:22, Jarin wrote:
> Remove comment.

Done.

https://codereview.chromium.org/2258073002/diff/40001/src/compiler/representa...
src/compiler/representation-change.cc:203: if (machine()->Is64() &&
output_type->Is(Type::Signed32())) {
On 2016/08/22 14:23:22, Jarin wrote:
> Do you ever get here? If this is a 64-bit platform, then Signed32 =
SignedSmall,
> so this case would be handled above.

Done.

https://codereview.chromium.org/2258073002/diff/40001/src/compiler/representa...
src/compiler/representation-change.cc:233: } else if (output_rep ==
MachineRepresentation::kBit) {
On 2016/08/22 14:23:22, Jarin wrote:
> This case is not necessary (since the final else goes to TypeError).

Done.

https://codereview.chromium.org/2258073002/diff/40001/src/compiler/representa...
src/compiler/representation-change.cc:242: // TaggedSigned? I think so because
we are on a 64bit platform.
On 2016/08/22 14:23:22, Jarin wrote:
> Yes, on 64-but platform this produces a Smi.

Thanks, removed TODO.

https://codereview.chromium.org/2258073002/diff/40001/src/compiler/representa...
src/compiler/representation-change.cc:261: return node;  // No change necessary.
On 2016/08/22 14:23:22, Jarin wrote:
> Actually, NumberConstant would be wrong because it could produce Smi.
> 
> I think for NumberConstants you should fall through.

Done.

https://codereview.chromium.org/2258073002/diff/40001/src/compiler/representa...
src/compiler/representation-change.cc:281: if
(output_type->Is(Type::SignedSmall())) {
On 2016/08/22 14:23:22, Jarin wrote:
> Here you have to ask if it can be a Smi:
> 
> if (output_type->Maybe(Type::SignedSmall())) {
> 

Done.

https://codereview.chromium.org/2258073002/diff/40001/src/compiler/representa...
src/compiler/representation-change.cc:622: if
(output_type->Is(Type::TaggedSigned())) {
On 2016/08/22 14:23:22, Jarin wrote:
> You should not need this case. Maybe you want to add a TODO reminder here.

Done.

https://codereview.chromium.org/2258073002/diff/40001/src/compiler/representa...
src/compiler/representation-change.cc:627: op =
simplified()->ChangeTaggedToInt32();
On 2016/08/22 14:23:22, Jarin wrote:
> This should be ChangeTaggedSignedToInt32, no?

Done.

https://codereview.chromium.org/2258073002/diff/40001/src/compiler/simplified...
File src/compiler/simplified-operator-reducer.cc (right):

https://codereview.chromium.org/2258073002/diff/40001/src/compiler/simplified...
src/compiler/simplified-operator-reducer.cc:71: Int32Matcher
m(node->InputAt(0));
On 2016/08/22 12:59:46, Michael Starzinger wrote:
> nit: Seems like a copy of the below, why the expansion?

Done.

[commit-bot: I haz the power, 2016-08-24 16:30:12]

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

[commit-bot: I haz the power, 2016-08-24 16:30:13]

Dry run: This issue passed the CQ dry run.

[Jarin, 2016-08-25 05:52:05]

https://codereview.chromium.org/2258073002/diff/100001/src/compiler/arm/instr...
File src/compiler/arm/instruction-selector-arm.cc (right):

https://codereview.chromium.org/2258073002/diff/100001/src/compiler/arm/instr...
src/compiler/arm/instruction-selector-arm.cc:435: rep ==
MachineRepresentation::kTaggedPointer);
Could you introduce some helper function in machine-type.h that encapsulates
this?

https://codereview.chromium.org/2258073002/diff/100001/src/compiler/represent...
File src/compiler/representation-change.cc (right):

https://codereview.chromium.org/2258073002/diff/100001/src/compiler/represent...
src/compiler/representation-change.cc:203: return
jsgraph()->Constant(static_cast<double>(value));
Actually, do we ever get here? I would think that the SignedSmall case already
handles this.

https://codereview.chromium.org/2258073002/diff/100001/src/compiler/represent...
src/compiler/representation-change.cc:258: return
jsgraph()->Constant(OpParameter<double>(node));
I do not think this guarantees non-smi. Can't we just do type error here? We
should not produce Float64Constant before representation inference. Same below.

https://codereview.chromium.org/2258073002/diff/100001/src/compiler/represent...
src/compiler/representation-change.cc:264: if (output_rep ==
MachineRepresentation::kTagged) {
Out of curiosity, do we ever get here?

https://codereview.chromium.org/2258073002/diff/100001/src/compiler/represent...
src/compiler/representation-change.cc:280: } else if (output_rep ==
MachineRepresentation::kBit) {
Do we ever get here?

https://codereview.chromium.org/2258073002/diff/100001/src/compiler/represent...
src/compiler/representation-change.cc:610: op =
simplified()->ChangeTaggedToUint32();
I am thinking we do not need the Unsigned case here, the signed case below
should handle this.

Actually, I would hope we need only the Signed case, the truncation to word32
should not matter - we should already have Signed32.

https://codereview.chromium.org/2258073002/diff/100001/src/compiler/represent...
src/compiler/representation-change.cc:619: } else if (use_info.type_check() ==
TypeCheckKind::kSignedSmall) {
We already have a Smi, so we should not even get here; same for Signed32 below.

[mvstanton, 2016-08-29 11:35:11]

The CQ bit was checked by mvstanton@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-29 11:35:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-29 11:59:51]

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

[commit-bot: I haz the power, 2016-08-29 11:59:52]

Dry run: This issue passed the CQ dry run.

[mvstanton, 2016-08-29 12:01:09]

The CQ bit was checked by mvstanton@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-29 12:01:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mvstanton, 2016-08-29 12:02:11]

Thanks again, Jaro. Comments addressed...
--Mike

https://codereview.chromium.org/2258073002/diff/100001/src/compiler/arm/instr...
File src/compiler/arm/instruction-selector-arm.cc (right):

https://codereview.chromium.org/2258073002/diff/100001/src/compiler/arm/instr...
src/compiler/arm/instruction-selector-arm.cc:435: rep ==
MachineRepresentation::kTaggedPointer);
On 2016/08/25 05:52:05, Jarin wrote:
> Could you introduce some helper function in machine-type.h that encapsulates
> this?

CanBeTaggedPointer(rep).

https://codereview.chromium.org/2258073002/diff/100001/src/compiler/represent...
File src/compiler/representation-change.cc (right):

https://codereview.chromium.org/2258073002/diff/100001/src/compiler/represent...
src/compiler/representation-change.cc:203: return
jsgraph()->Constant(static_cast<double>(value));
On 2016/08/25 05:52:05, Jarin wrote:
> Actually, do we ever get here? I would think that the SignedSmall case already
> handles this.

Done.

https://codereview.chromium.org/2258073002/diff/100001/src/compiler/represent...
src/compiler/representation-change.cc:258: return
jsgraph()->Constant(OpParameter<double>(node));
On 2016/08/25 05:52:05, Jarin wrote:
> I do not think this guarantees non-smi. Can't we just do type error here? We
> should not produce Float64Constant before representation inference. Same
below.

Good point, this and kFloat32Constant now would produce a type error.

https://codereview.chromium.org/2258073002/diff/100001/src/compiler/represent...
src/compiler/representation-change.cc:264: if (output_rep ==
MachineRepresentation::kTagged) {
On 2016/08/25 05:52:05, Jarin wrote:
> Out of curiosity, do we ever get here?

Nope, I've removed it, thanks for the general thrust away from comprehensive
"what-ifs" to simply doing what we need.

https://codereview.chromium.org/2258073002/diff/100001/src/compiler/represent...
src/compiler/representation-change.cc:280: } else if (output_rep ==
MachineRepresentation::kBit) {
On 2016/08/25 05:52:05, Jarin wrote:
> Do we ever get here?

No, I've removed the case.

https://codereview.chromium.org/2258073002/diff/100001/src/compiler/represent...
src/compiler/representation-change.cc:610: op =
simplified()->ChangeTaggedToUint32();
On 2016/08/25 05:52:05, Jarin wrote:
> I am thinking we do not need the Unsigned case here, the signed case below
> should handle this.
> 
> Actually, I would hope we need only the Signed case, the truncation to word32
> should not matter - we should already have Signed32.

Done.

https://codereview.chromium.org/2258073002/diff/100001/src/compiler/represent...
src/compiler/representation-change.cc:619: } else if (use_info.type_check() ==
TypeCheckKind::kSignedSmall) {
On 2016/08/25 05:52:05, Jarin wrote:
> We already have a Smi, so we should not even get here; same for Signed32
below.

Done.

https://codereview.chromium.org/2258073002/diff/120001/src/compiler/code-gene...
File src/compiler/code-generator.cc (right):

https://codereview.chromium.org/2258073002/diff/120001/src/compiler/code-gene...
src/compiler/code-generator.cc:825: type.representation() ==
MachineRepresentation::kTaggedPointer) {
Introduced helper function for these cases, IsAnyTagged(MachineRepresentation).

[Jarin, 2016-08-29 12:09:20]

lgtm. Thanks!

[commit-bot: I haz the power, 2016-08-29 12:29:40]

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

[commit-bot: I haz the power, 2016-08-29 12:29:41]

Dry run: This issue passed the CQ dry run.

[mvstanton, 2016-08-29 12:30:15]

The CQ bit was checked by mvstanton@chromium.org

[commit-bot: I haz the power, 2016-08-29 12:30:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-29 12:32:38]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2016-08-29 12:33:25]

Description was changed from

==========
[Turbofan]: Use new MachineTypes in access-builder.

Introduced MachineType::TaggedSigned() and TaggedPointer().

The idea is to quit using the representational dimension of Type, and
instead encode this information in the MachineRepresentation (itself
lightly wrapped in MachineType, along with MachineSemantic).

There are three parts to the whole change:

1) Places that set the machine representation - constant nodes, loads nad
   stores, global object and native context specialization.

2) Places that propagate type/representation - this is representation
   inference (aka simplified lowering). At the end of this process we
   expect to have a MachineRepresentation for every node. An interesting
   part of this is phi merging.

3) Places that examine representation - WriteBarrier elimination does this.
   Currently it's looking at the Type representation dimension, but as
   a part of this change (or in a soon-to-follow change) it can simply
   examine the MachineRepresentation.

BUG=
==========

to

==========
[Turbofan]: Use new MachineTypes in access-builder.

Introduced MachineType::TaggedSigned() and TaggedPointer().

The idea is to quit using the representational dimension of Type, and
instead encode this information in the MachineRepresentation (itself
lightly wrapped in MachineType, along with MachineSemantic).

There are three parts to the whole change:

1) Places that set the machine representation - constant nodes, loads nad
   stores, global object and native context specialization.

2) Places that propagate type/representation - this is representation
   inference (aka simplified lowering). At the end of this process we
   expect to have a MachineRepresentation for every node. An interesting
   part of this is phi merging.

3) Places that examine representation - WriteBarrier elimination does this.
   Currently it's looking at the Type representation dimension, but as
   a part of this change (or in a soon-to-follow change) it can simply
   examine the MachineRepresentation.

BUG=

Committed: https://crrev.com/56429fc14671a10749190a4dfeacd38b7270f6f5
Cr-Commit-Position: refs/heads/master@{#38978}
==========

[commit-bot: I haz the power, 2016-08-29 12:33:26]

Patchset 7 (id:??) landed as
https://crrev.com/56429fc14671a10749190a4dfeacd38b7270f6f5
Cr-Commit-Position: refs/heads/master@{#38978}