[Turbofan] Add the concept of aliasing to RegisterConfiguration.

[Turbofan] Add the concept of aliasing to RegisterConfiguration.
- Adds the concept of FP register aliasing to RegisterConfiguration.
- Changes RegisterAllocator to distinguish between FP representations
when allocating.
- Changes LinearScanAllocator to detect interference when FP register
aliasing is combining, as on ARM.
- Changes ARM code generation to allow all registers s0 - s31 to be
accessed.
- Adds unit tests for RegisterConfiguration, mostly to test aliasing
calculations.

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

[bbudge, 2016-06-21 23:09:34]

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

[bbudge, 2016-06-21 23:09:35]

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

https://codereview.chromium.org/2086653003/diff/60001/src/compiler/register-a...
src/compiler/register-allocator.cc:2078: }
Mircea, I still think I need use positions for fixed float live ranges, since
the float operands don't canonicalize the same as double operands any more. So
RegisterAllocatorVerifier won't find a use for a move with a float operand
following a call.

https://codereview.chromium.org/2086653003/diff/60001/src/compiler/register-a...
File src/compiler/register-allocator.h (left):

https://codereview.chromium.org/2086653003/diff/60001/src/compiler/register-a...
src/compiler/register-allocator.h:682: int ByteWidth() const;
This is redundant, as a byte_width() getter is defined below.

[Benedikt Meurer, 2016-06-22 04:43:37]

bmeurer@chromium.org changed reviewers:
+ jarin@chromium.org

[Mircea Trofin, 2016-06-22 06:07:35]

https://codereview.chromium.org/2086653003/diff/60001/src/compiler/arm/code-g...
File src/compiler/arm/code-generator-arm.cc (right):

https://codereview.chromium.org/2086653003/diff/60001/src/compiler/arm/code-g...
src/compiler/arm/code-generator-arm.cc:38: SwVfpRegister
ToFloat32Register(InstructionOperand* op) {
If this is only called for LocationOperands, should we change the signature
accordingly?

https://codereview.chromium.org/2086653003/diff/60001/src/compiler/instructio...
File src/compiler/instruction.cc (right):

https://codereview.chromium.org/2086653003/diff/60001/src/compiler/instructio...
src/compiler/instruction.cc:65: const LocationOperand& loc1 =
*LocationOperand::cast(this);
I'd move this method to LocationOperand and change the type of "that", too,
accordingly

https://codereview.chromium.org/2086653003/diff/60001/src/compiler/instructio...
src/compiler/instruction.cc:68:
RegisterConfiguration::ArchDefault(RegisterConfiguration::TURBOFAN);
Why force the configuration to TURBOFAN? How do unittests pass?

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

https://codereview.chromium.org/2086653003/diff/60001/src/compiler/register-a...
src/compiler/register-allocator.cc:2078: }
On 2016/06/21 23:09:35, bbudge wrote:
> Mircea, I still think I need use positions for fixed float live ranges, since
> the float operands don't canonicalize the same as double operands any more. So
> RegisterAllocatorVerifier won't find a use for a move with a float operand
> following a call.

Wouldn't the virtual register that the output gets assigned to be of the same
size as the output register?

https://codereview.chromium.org/2086653003/diff/60001/src/compiler/register-a...
src/compiler/register-allocator.cc:2978: int alias_base_index;
Could you initialize this?

https://codereview.chromium.org/2086653003/diff/60001/src/compiler/register-a...
src/compiler/register-allocator.cc:3014: int alias_base_index;
please initialize

https://codereview.chromium.org/2086653003/diff/60001/src/compiler/register-a...
File src/compiler/register-allocator.h (left):

https://codereview.chromium.org/2086653003/diff/60001/src/compiler/register-a...
src/compiler/register-allocator.h:682: int ByteWidth() const;
On 2016/06/21 23:09:35, bbudge wrote:
> This is redundant, as a byte_width() getter is defined below.

Acknowledged.

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

https://codereview.chromium.org/2086653003/diff/60001/src/compiler/register-a...
src/compiler/register-allocator.h:967: bool no_combining() const { return
no_combining_; }
could you add a comment as to what this represents? Should it be no_aliasing
rather?

https://codereview.chromium.org/2086653003/diff/60001/src/register-configurat...
File src/register-configuration.cc (right):

https://codereview.chromium.org/2086653003/diff/60001/src/register-configurat...
src/register-configuration.cc:181: int
RegisterConfiguration::GetAliases(MachineRepresentation rep, int index,
For readability, instead of return value + out val, could you alternatively
return a std::pair, or a structure with friendly fields?

[bbudge, 2016-06-22 20:16:04]

https://codereview.chromium.org/2086653003/diff/60001/src/compiler/arm/code-g...
File src/compiler/arm/code-generator-arm.cc (right):

https://codereview.chromium.org/2086653003/diff/60001/src/compiler/arm/code-g...
src/compiler/arm/code-generator-arm.cc:38: SwVfpRegister
ToFloat32Register(InstructionOperand* op) {
On 2016/06/22 06:07:34, Mircea Trofin wrote:
> If this is only called for LocationOperands, should we change the signature
> accordingly?

I followed the pattern of ToFloat64Register. Changing the signature would
require casts at all the call sites (there are about 10 for both functions).

https://codereview.chromium.org/2086653003/diff/60001/src/compiler/instructio...
File src/compiler/instruction.cc (right):

https://codereview.chromium.org/2086653003/diff/60001/src/compiler/instructio...
src/compiler/instruction.cc:65: const LocationOperand& loc1 =
*LocationOperand::cast(this);
On 2016/06/22 06:07:34, Mircea Trofin wrote:
> I'd move this method to LocationOperand and change the type of "that", too,
> accordingly

I agree it doesn't feel natural for this to be on InstructionOperand.

However, if this is moved to LocationOperand, then the two call sites have to be
modified to test for IsLocation or IsFPRegister on both operands (calling
EqualsCanonicalized on failure). We would still have to test in the method that
both operands are FP regs and call EqualsCanonicalized there as well on failure.
It's a bit more code and more testing at runtime.

WDYT?

https://codereview.chromium.org/2086653003/diff/60001/src/compiler/instructio...
src/compiler/instruction.cc:68:
RegisterConfiguration::ArchDefault(RegisterConfiguration::TURBOFAN);
On 2016/06/22 06:07:34, Mircea Trofin wrote:
> Why force the configuration to TURBOFAN? How do unittests pass?

We want the global register configuration for Turbofan here. It knows about the
actual registers and aliasing for the target platform.

The unit tests create their own test instances of RegisterConfiguration (they
don't test the global instances for Turbofan or Crankshaft, though such tests
are possible.)

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

https://codereview.chromium.org/2086653003/diff/60001/src/compiler/register-a...
src/compiler/register-allocator.cc:2078: }
On 2016/06/22 06:07:34, Mircea Trofin wrote:
> On 2016/06/21 23:09:35, bbudge wrote:
> > Mircea, I still think I need use positions for fixed float live ranges,
since
> > the float operands don't canonicalize the same as double operands any more.
So
> > RegisterAllocatorVerifier won't find a use for a move with a float operand
> > following a call.
> 
> Wouldn't the virtual register that the output gets assigned to be of the same
> size as the output register?

In RegisterAllocatorVerifier::VerifyGapMoves, allocated operands are used as
keys (so register index is used, not virtual register number). Since fp operands
with different reps never canonicalize to the same value anymore, this causes
various map lookups to fail. In particular, this is hit:

https://cs-staging.chromium.org/chromium/src/v8/src/compiler/register-allocat...

during test/mjsunit/wasm/gc-frame.js and the operand on ARM is register s1
(f32). This second loop is needed to prevent that.

https://codereview.chromium.org/2086653003/diff/60001/src/compiler/register-a...
src/compiler/register-allocator.cc:2978: int alias_base_index;
On 2016/06/22 06:07:34, Mircea Trofin wrote:
> Could you initialize this?

Done (at all call sites - there are 6)

https://codereview.chromium.org/2086653003/diff/60001/src/compiler/register-a...
src/compiler/register-allocator.cc:3014: int alias_base_index;
On 2016/06/22 06:07:34, Mircea Trofin wrote:
> please initialize

Done.

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

https://codereview.chromium.org/2086653003/diff/60001/src/compiler/register-a...
src/compiler/register-allocator.h:967: bool no_combining() const { return
no_combining_; }
On 2016/06/22 06:07:34, Mircea Trofin wrote:
> could you add a comment as to what this represents? Should it be no_aliasing
> rather?

Done.

https://codereview.chromium.org/2086653003/diff/60001/src/register-configurat...
File src/register-configuration.cc (right):

https://codereview.chromium.org/2086653003/diff/60001/src/register-configurat...
src/register-configuration.cc:181: int
RegisterConfiguration::GetAliases(MachineRepresentation rep, int index,
On 2016/06/22 06:07:34, Mircea Trofin wrote:
> For readability, instead of return value + out val, could you alternatively
> return a std::pair, or a structure with friendly fields?

I think std::pair is arguably less readable (first, second rather than
descriptive names of int fields.)

I tried a struct but it's also awkward, since the call sites use the returned
number of aliases as a loop counter. Using a struct makes those loops strange,
e.g.

  RegisterConfiguration::AliasInfo aliases = config()->GetAliases(...
  while (alias.count--) {
    int aliased_reg = alias.base + alias.count;  // <- strangeness

I think this idiom (return value plus *optional* out param) is fairly
mainstream.

[bbudge, 2016-06-22 20:28:10]

A few other issues I think are worth discussing:

- The RegisterConfiguration takes an fp_aliasing_kind parameter, which can be
queried at runtime by RegisterAllocator to determine if the more complicated
aliasing calculations are needed. However, for most platforms, these
calculations are never done (e.g. Intel). Since we know this at compile time, it
would be possible to #ifdef so that Intel had no code size or compile speed
impact. This would require each arch to define a symbol for the #ifdefs, and
some platform-specific stuff in RegisterAllocator, MoveOptimizer and a few other
places.

- In a previous CL, I changed InstructionOperand canonicalization to preserve
the FP representation for all platforms. This is really only necessary on ARM
and other platforms with "combine" aliasing. We could use the same #define as
above to keep Intel and others unchanged. It seemed strange to me that
LiveRanges with different reps could be considered equivalent, but there are
advantages to that. For example, we wouldn't have to add use positions to
float32 fixed live ranges around call instructions.

[Mircea Trofin, 2016-06-22 21:28:17]

On 2016/06/22 20:28:10, bbudge wrote:
> A few other issues I think are worth discussing:
> 
> - The RegisterConfiguration takes an fp_aliasing_kind parameter, which can be
> queried at runtime by RegisterAllocator to determine if the more complicated
> aliasing calculations are needed. However, for most platforms, these
> calculations are never done (e.g. Intel). Since we know this at compile time,
it
> would be possible to #ifdef so that Intel had no code size or compile speed
> impact. This would require each arch to define a symbol for the #ifdefs, and
> some platform-specific stuff in RegisterAllocator, MoveOptimizer and a few
other
> places.
> 

You may be able to avoid having platform specific code in those places if the
test is
on a (for example) kTargetHasRegisterAliasing constant. This constant would be
only
true on arm, of course. Since it's a constant, it's likely the compiler will
elide 
the never-taken conditional branches.

> - In a previous CL, I changed InstructionOperand canonicalization to preserve
> the FP representation for all platforms. This is really only necessary on ARM
> and other platforms with "combine" aliasing. We could use the same #define as
> above to keep Intel and others unchanged. It seemed strange to me that
> LiveRanges with different reps could be considered equivalent, but there are
> advantages to that. For example, we wouldn't have to add use positions to
> float32 fixed live ranges around call instructions.

Re. the last sentence: you mean, no need to add both float64 and float32 use
positions, do I understand that correctly?

[Mircea Trofin, 2016-06-22 21:37:37]

https://codereview.chromium.org/2086653003/diff/60001/src/compiler/instructio...
File src/compiler/instruction.cc (right):

https://codereview.chromium.org/2086653003/diff/60001/src/compiler/instructio...
src/compiler/instruction.cc:68:
RegisterConfiguration::ArchDefault(RegisterConfiguration::TURBOFAN);
On 2016/06/22 20:16:04, bbudge wrote:
> On 2016/06/22 06:07:34, Mircea Trofin wrote:
> > Why force the configuration to TURBOFAN? How do unittests pass?
> 
> We want the global register configuration for Turbofan here. It knows about
the
> actual registers and aliasing for the target platform.
> 
> The unit tests create their own test instances of RegisterConfiguration (they
> don't test the global instances for Turbofan or Crankshaft, though such tests
> are possible.)

Not super sure I understand. Are you saying that aliasing is only a Turbofan
concept? If so, then should we just check if we're running under a turbofan
config?

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

https://codereview.chromium.org/2086653003/diff/60001/src/compiler/register-a...
src/compiler/register-allocator.h:967: bool no_combining() const { return
no_combining_; }
On 2016/06/22 20:16:04, bbudge wrote:
> On 2016/06/22 06:07:34, Mircea Trofin wrote:
> > could you add a comment as to what this represents? Should it be no_aliasing
> > rather?
> 
> Done.

Where?

[bbudge, 2016-06-22 21:51:14]

https://codereview.chromium.org/2086653003/diff/60001/src/compiler/instructio...
File src/compiler/instruction.cc (right):

https://codereview.chromium.org/2086653003/diff/60001/src/compiler/instructio...
src/compiler/instruction.cc:68:
RegisterConfiguration::ArchDefault(RegisterConfiguration::TURBOFAN);
On 2016/06/22 21:37:37, Mircea Trofin wrote:
> On 2016/06/22 20:16:04, bbudge wrote:
> > On 2016/06/22 06:07:34, Mircea Trofin wrote:
> > > Why force the configuration to TURBOFAN? How do unittests pass?
> > 
> > We want the global register configuration for Turbofan here. It knows about
> the
> > actual registers and aliasing for the target platform.
> > 
> > The unit tests create their own test instances of RegisterConfiguration
(they
> > don't test the global instances for Turbofan or Crankshaft, though such
tests
> > are possible.)
> 
> Not super sure I understand. Are you saying that aliasing is only a Turbofan
> concept? If so, then should we just check if we're running under a turbofan
> config?

Outside of tests we should use either the Turbofan or Crankshaft configs. They
are currently identical, except for the number of aliased double registers. I
use the Turbofan config because this is in src/compiler.

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

https://codereview.chromium.org/2086653003/diff/60001/src/compiler/register-a...
src/compiler/register-allocator.h:967: bool no_combining() const { return
no_combining_; }
On 2016/06/22 21:37:37, Mircea Trofin wrote:
> On 2016/06/22 20:16:04, bbudge wrote:
> > On 2016/06/22 06:07:34, Mircea Trofin wrote:
> > > could you add a comment as to what this represents? Should it be
no_aliasing
> > > rather?
> > 
> > Done.
> 
> Where?

See the next revision. As for your question, I considered no_aliasing() but
decided that since all platforms have aliasing (OVERLAP for Intel, COMBINE for
ARM), that this was preferable. Note that no_combine() is always true when
allocating general regs.

[bbudge, 2016-06-23 01:19:08]

On 2016/06/22 21:28:17, Mircea Trofin wrote:
> On 2016/06/22 20:28:10, bbudge wrote:
> > A few other issues I think are worth discussing:
> > 
> > - The RegisterConfiguration takes an fp_aliasing_kind parameter, which can
be
> > queried at runtime by RegisterAllocator to determine if the more complicated
> > aliasing calculations are needed. However, for most platforms, these
> > calculations are never done (e.g. Intel). Since we know this at compile
time,
> it
> > would be possible to #ifdef so that Intel had no code size or compile speed
> > impact. This would require each arch to define a symbol for the #ifdefs, and
> > some platform-specific stuff in RegisterAllocator, MoveOptimizer and a few
> other
> > places.
> > 
> 
> You may be able to avoid having platform specific code in those places if the
> test is
> on a (for example) kTargetHasRegisterAliasing constant. This constant would be
> only
> true on arm, of course. Since it's a constant, it's likely the compiler will
> elide 
> the never-taken conditional branches.
> 
> > - In a previous CL, I changed InstructionOperand canonicalization to
preserve
> > the FP representation for all platforms. This is really only necessary on
ARM
> > and other platforms with "combine" aliasing. We could use the same #define
as
> > above to keep Intel and others unchanged. It seemed strange to me that
> > LiveRanges with different reps could be considered equivalent, but there are
> > advantages to that. For example, we wouldn't have to add use positions to
> > float32 fixed live ranges around call instructions.
> 
> Re. the last sentence: you mean, no need to add both float64 and float32 use
> positions, do I understand that correctly?

Correct.

[Mircea Trofin, 2016-06-23 19:34:45]

On 2016/06/23 01:19:08, bbudge wrote:
> On 2016/06/22 21:28:17, Mircea Trofin wrote:
> > On 2016/06/22 20:28:10, bbudge wrote:
> > > A few other issues I think are worth discussing:
> > > 
> > > - The RegisterConfiguration takes an fp_aliasing_kind parameter, which can
> be
> > > queried at runtime by RegisterAllocator to determine if the more
complicated
> > > aliasing calculations are needed. However, for most platforms, these
> > > calculations are never done (e.g. Intel). Since we know this at compile
> time,
> > it
> > > would be possible to #ifdef so that Intel had no code size or compile
speed
> > > impact. This would require each arch to define a symbol for the #ifdefs,
and
> > > some platform-specific stuff in RegisterAllocator, MoveOptimizer and a few
> > other
> > > places.
> > > 
> > 
> > You may be able to avoid having platform specific code in those places if
the
> > test is
> > on a (for example) kTargetHasRegisterAliasing constant. This constant would
be
> > only
> > true on arm, of course. Since it's a constant, it's likely the compiler will
> > elide 
> > the never-taken conditional branches.
> > 
> > > - In a previous CL, I changed InstructionOperand canonicalization to
> preserve
> > > the FP representation for all platforms. This is really only necessary on
> ARM
> > > and other platforms with "combine" aliasing. We could use the same #define
> as
> > > above to keep Intel and others unchanged. It seemed strange to me that
> > > LiveRanges with different reps could be considered equivalent, but there
are
> > > advantages to that. For example, we wouldn't have to add use positions to
> > > float32 fixed live ranges around call instructions.
> > 
> > Re. the last sentence: you mean, no need to add both float64 and float32 use
> > positions, do I understand that correctly?
> 
> Correct.

lgtm

[Benedikt Meurer, 2016-06-24 03:33:39]

LGTM (mostly rubber-stamped)

[bbudge, 2016-06-24 12:32:21]

The CQ bit was checked by bbudge@chromium.org

[bbudge, 2016-06-24 12:32:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, mtrofin@chromium.org
Link to the patchset: https://codereview.chromium.org/2086653003/#ps120001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-06-24 12:32:25]

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

[bbudge, 2016-06-24 12:44:04]

The CQ bit was unchecked by bbudge@chromium.org

[commit-bot: I haz the power, 2016-06-24 12:44:12]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-06-24 12:45:14]

Description was changed from

==========
[Turbofan] Add the concept of aliasing to RegisterConfiguration.
- Adds the concept of FP register aliasing to RegisterConfiguration.
- Changes RegisterAllocator to distinguish between FP representations
when allocating.
- Changes LinearScanAllocator to detect interference when FP register
aliasing is combining, as on ARM.
- Changes ARM code generation to allow all registers s0 - s31 to be
accessed.
- Adds unit tests for RegisterConfiguration, mostly to test aliasing
calculations.

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

to

==========
[Turbofan] Add the concept of aliasing to RegisterConfiguration.
- Adds the concept of FP register aliasing to RegisterConfiguration.
- Changes RegisterAllocator to distinguish between FP representations
when allocating.
- Changes LinearScanAllocator to detect interference when FP register
aliasing is combining, as on ARM.
- Changes ARM code generation to allow all registers s0 - s31 to be
accessed.
- Adds unit tests for RegisterConfiguration, mostly to test aliasing
calculations.

LOG=N
BUG=v8:4124

Committed: https://crrev.com/a933b7044a76d99c84607c846487590413cf6743
Cr-Commit-Position: refs/heads/master@{#37251}
==========

[commit-bot: I haz the power, 2016-06-24 12:45:15]

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

[Michael Achenbach, 2016-06-27 07:57:22]

machenbach@chromium.org changed reviewers:
+ machenbach@chromium.org

[Michael Achenbach, 2016-06-27 07:57:23]

https://codereview.chromium.org/2086653003/diff/120001/test/unittests/unittes...
File test/unittests/unittests.gyp (right):

https://codereview.chromium.org/2086653003/diff/120001/test/unittests/unittes...
test/unittests/unittests.gyp:123: 'register-configuration-unittest.cc',
Always keep BUILD.gn in sync. Otherwise the test will stop running soon... will
upload a fix.