[Turbofan] Merge SpillRanges by byte_width rather than kind.

[Turbofan] Merge SpillRanges by byte_width rather than kind.
- Uses byte_width() to determine if spill ranges can be merged.
- Modifies InstructionOperand canonicalization to ignore representation for stack slots.

LOG=N
BUG=v8:4124
Committed: https://crrev.com/55841409c01f2c929f4e584cf77273346c006ad2
Cr-Commit-Position: refs/heads/master@{#37463}

[bbudge, 2016-06-18 23:14:52]

bbudge@chromium.org changed reviewers:
+ mtrofin@chromium.org

[Mircea Trofin, 2016-06-18 23:27:56]

Lgtm, some nits

https://codereview.chromium.org/2074323002/diff/1/src/compiler/graph-visualiz...
File src/compiler/graph-visualizer.cc (right):

https://codereview.chromium.org/2074323002/diff/1/src/compiler/graph-visualiz...
src/compiler/graph-visualizer.cc:564:
DCHECK(!IsFloatingPoint(top->representation()));
When would this check fail?

https://codereview.chromium.org/2074323002/diff/1/src/compiler/register-alloc...
File src/compiler/register-allocator.h (right):

https://codereview.chromium.org/2074323002/diff/1/src/compiler/register-alloc...
src/compiler/register-allocator.h:699: MachineRepresentation representation()
const { return representation_; }
Check indentation here (may be just something weird in my browser)

[bbudge, 2016-06-18 23:33:23]

Thanks for looking. I promise to stop working on the weekend.

https://codereview.chromium.org/2074323002/diff/1/src/compiler/graph-visualiz...
File src/compiler/graph-visualizer.cc (right):

https://codereview.chromium.org/2074323002/diff/1/src/compiler/graph-visualiz...
src/compiler/graph-visualizer.cc:564:
DCHECK(!IsFloatingPoint(top->representation()));
On 2016/06/18 23:27:56, Mircea Trofin wrote:
> When would this check fail?

Yes, that's kind of dumb. Removed.

https://codereview.chromium.org/2074323002/diff/1/src/compiler/register-alloc...
File src/compiler/register-allocator.cc (right):

https://codereview.chromium.org/2074323002/diff/1/src/compiler/register-alloc...
src/compiler/register-allocator.cc:1269:
other->live_ranges_[0]->representation() ||
It occurs to me that this will now prevent non-FP spill ranges from being
merged. Could that cause a problem? The comment I deleted implies that this
should be possible, at least for non-FP types which are all different sizes.

https://codereview.chromium.org/2074323002/diff/1/src/compiler/register-alloc...
File src/compiler/register-allocator.h (right):

https://codereview.chromium.org/2074323002/diff/1/src/compiler/register-alloc...
src/compiler/register-allocator.h:699: MachineRepresentation representation()
const { return representation_; }
On 2016/06/18 23:27:56, Mircea Trofin wrote:
> Check indentation here (may be just something weird in my browser)

Seems OK.

[Mircea Trofin, 2016-06-18 23:43:35]

On 2016/06/18 23:33:23, bbudge wrote:
> Thanks for looking. I promise to stop working on the weekend.
> 
>
https://codereview.chromium.org/2074323002/diff/1/src/compiler/graph-visualiz...
> File src/compiler/graph-visualizer.cc (right):
> 
>
https://codereview.chromium.org/2074323002/diff/1/src/compiler/graph-visualiz...
> src/compiler/graph-visualizer.cc:564:
> DCHECK(!IsFloatingPoint(top->representation()));
> On 2016/06/18 23:27:56, Mircea Trofin wrote:
> > When would this check fail?
> 
> Yes, that's kind of dumb. Removed.
> 
>
https://codereview.chromium.org/2074323002/diff/1/src/compiler/register-alloc...
> File src/compiler/register-allocator.cc (right):
> 
>
https://codereview.chromium.org/2074323002/diff/1/src/compiler/register-alloc...
> src/compiler/register-allocator.cc:1269:
> other->live_ranges_[0]->representation() ||
> It occurs to me that this will now prevent non-FP spill ranges from being
> merged. Could that cause a problem? The comment I deleted implies that this
> should be possible, at least for non-FP types which are all different sizes.
> 

That would cause a regression. We should test for that. Also, could you run the
benchmarks?

>
https://codereview.chromium.org/2074323002/diff/1/src/compiler/register-alloc...
> File src/compiler/register-allocator.h (right):
> 
>
https://codereview.chromium.org/2074323002/diff/1/src/compiler/register-alloc...
> src/compiler/register-allocator.h:699: MachineRepresentation representation()
> const { return representation_; }
> On 2016/06/18 23:27:56, Mircea Trofin wrote:
> > Check indentation here (may be just something weird in my browser)
> 
> Seems OK.

[bbudge, 2016-06-30 23:36:11]

Description was changed from

==========
[Turbofan] Maintain SpillRange representation instead of kind.
Eliminates redundant SpillRange::ByteWidth() method.

LOG=N
BUG=v8:4124
==========

to

==========
[Turbofan] Merge SpillRanges by byte_width rather than kind.
- Uses byte_width() to determine if spill ranges can be merged.
- Modifies InstructionOperand canonicalization to ignore representation for
stack slots.

LOG=N
BUG=v8:4124
==========

[bbudge, 2016-06-30 23:54:19]

This has changed enough that I think it should have another review. This is much
better than before. SpillRange shouldn't have a representation (because ranges
of different reps can be merged).

This now effects dcarney's original TODO to use byte_width for merging.

[Mircea Trofin, 2016-06-30 23:58:29]

lgtm

https://codereview.chromium.org/2074323002/diff/80001/src/compiler/register-a...
File src/compiler/register-allocator.cc (right):

https://codereview.chromium.org/2074323002/diff/80001/src/compiler/register-a...
src/compiler/register-allocator.cc:1236: if (byte_width() != other->byte_width()
|| IsIntersectingWith(other))
since the return is on the next line, please put {}

https://codereview.chromium.org/2074323002/diff/80001/src/compiler/register-a...
File src/compiler/register-allocator.h (right):

https://codereview.chromium.org/2074323002/diff/80001/src/compiler/register-a...
src/compiler/register-allocator.h:698: // Currently, only 4 or 8 byte slots are
supported.
is this a //TODO ?

[bbudge, 2016-07-01 00:12:07]

https://codereview.chromium.org/2074323002/diff/80001/src/compiler/register-a...
File src/compiler/register-allocator.h (right):

https://codereview.chromium.org/2074323002/diff/80001/src/compiler/register-a...
src/compiler/register-allocator.h:698: // Currently, only 4 or 8 byte slots are
supported.
On 2016/06/30 23:58:29, Mircea Trofin wrote:
> is this a //TODO ?

It's kind of strange, since byte_width can actually be 16 for SIMD. I added a
TODO for supporting that in frames.h.

[bbudge, 2016-07-01 00:16:23]

The CQ bit was checked by bbudge@chromium.org

[bbudge, 2016-07-01 00:16:23]

The patchset sent to the CQ was uploaded after l-g-t-m from mtrofin@chromium.org
Link to the patchset: https://codereview.chromium.org/2074323002/#ps100001
(title: "Review comments.")

[commit-bot: I haz the power, 2016-07-01 00:16:31]

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-07-01 00:56:10]

Description was changed from

==========
[Turbofan] Merge SpillRanges by byte_width rather than kind.
- Uses byte_width() to determine if spill ranges can be merged.
- Modifies InstructionOperand canonicalization to ignore representation for
stack slots.

LOG=N
BUG=v8:4124
==========

to

==========
[Turbofan] Merge SpillRanges by byte_width rather than kind.
- Uses byte_width() to determine if spill ranges can be merged.
- Modifies InstructionOperand canonicalization to ignore representation for
stack slots.

LOG=N
BUG=v8:4124
==========

[commit-bot: I haz the power, 2016-07-01 00:56:11]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-07-01 00:57:13]

Description was changed from

==========
[Turbofan] Merge SpillRanges by byte_width rather than kind.
- Uses byte_width() to determine if spill ranges can be merged.
- Modifies InstructionOperand canonicalization to ignore representation for
stack slots.

LOG=N
BUG=v8:4124
==========

to

==========
[Turbofan] Merge SpillRanges by byte_width rather than kind.
- Uses byte_width() to determine if spill ranges can be merged.
- Modifies InstructionOperand canonicalization to ignore representation for
stack slots.

LOG=N
BUG=v8:4124

Committed: https://crrev.com/55841409c01f2c929f4e584cf77273346c006ad2
Cr-Commit-Position: refs/heads/master@{#37463}
==========

[commit-bot: I haz the power, 2016-07-01 00:57:14]

Patchset 6 (id:??) landed as
https://crrev.com/55841409c01f2c929f4e584cf77273346c006ad2
Cr-Commit-Position: refs/heads/master@{#37463}