[Turbofan] Add concept of FP register aliasing on ARM 32.

[Turbofan] Add concept of FP register aliasing on ARM 32.

- Modifies RegisterConfiguration to specify complex aliasing on ARM 32.
- Modifies RegisterAllocator to consider aliasing.
- Modifies ParallelMove::PrepareInsertAfter to handle aliasing.
- Modifies GapResolver to split wider register moves when interference
with smaller moves is detected.
- Modifies MoveOptimizer to handle aliasing.
- Adds ARM 32 macro-assembler pseudo move instructions to handle cases where
  split moves don't correspond to actual s-registers.
- Modifies CodeGenerator::AssembleMove and AssembleSwap to handle moves of
  different widths, and moves involving pseudo-s-registers.
- Adds unit tests for FP operand interference checking and PrepareInsertAfter.
- Adds more tests of FP for the move optimizer and register allocator.

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

[bbudge, 2016-10-11 00:53:44]

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

[commit-bot: I haz the power, 2016-10-11 00:53:52]

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-10-11 00:59:01]

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

[commit-bot: I haz the power, 2016-10-11 00:59:02]

Dry run: Try jobs failed on following builders:
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/14273)
  v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/10482)

[bbudge, 2016-10-11 01:15:26]

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

[commit-bot: I haz the power, 2016-10-11 01:15:30]

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-10-11 01:23:14]

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

[commit-bot: I haz the power, 2016-10-11 01:23:15]

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

[bbudge, 2016-10-11 01:28:12]

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

[commit-bot: I haz the power, 2016-10-11 01:28:20]

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-10-11 01:50:48]

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

[commit-bot: I haz the power, 2016-10-11 01:50:49]

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

[bbudge, 2016-10-11 14:47:17]

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

[commit-bot: I haz the power, 2016-10-11 14:47:29]

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-10-11 15:06:59]

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

[commit-bot: I haz the power, 2016-10-11 15:07:00]

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/10546)
  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...)

[bbudge, 2016-10-11 15:45:34]

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

[commit-bot: I haz the power, 2016-10-11 15:45:42]

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-10-11 16:08:12]

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

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

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

[bbudge, 2016-10-11 18:27:36]

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

[commit-bot: I haz the power, 2016-10-11 18:27:45]

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-10-11 19:18:16]

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

[commit-bot: I haz the power, 2016-10-11 19:18:18]

Dry run: This issue passed the CQ dry run.

[bbudge, 2016-10-11 19:57:16]

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

[bbudge, 2016-10-11 19:57:17]

Benedikt / Jarin for ARM assembler / macroassembler

Mircea for everything else.

[bbudge, 2016-10-11 20:26:49]

cc'ing Danno (you're welcome to review!) and Georgia (feel free to review or
leave comments.)

[georgia.kouveli, 2016-10-12 17:38:41]

https://codereview.chromium.org/2410673002/diff/100001/src/compiler/gap-resol...
File src/compiler/gap-resolver.cc (right):

https://codereview.chromium.org/2410673002/diff/100001/src/compiler/gap-resol...
src/compiler/gap-resolver.cc:203: // Ensure source is a register, to limit swap
cases.
Isn't it still possible for both source and register to be stack slots, as in
the old comment?

https://codereview.chromium.org/2410673002/diff/100001/src/compiler/instructi...
File src/compiler/instruction.cc (right):

https://codereview.chromium.org/2410673002/diff/100001/src/compiler/instructi...
src/compiler/instruction.cc:277: to_eliminate->push_back(curr);
If the representation of "move" is narrower than the representation of "curr",
shouldn't this split "curr" like in the gap resolver instead of removing it?

I'm thinking of the case where the existing parallel move contains the move:
D1 -> D0
and you want to add:
S4 -> S0
where eliminating D1->D0 and just keeping S4->S0 doesn't have the same semantics
as performing S4->S0 after D1->D0.

The "if" case (destination interfering with source) seems more complicated, you
could have:
D1 -> D0
and add:
S0 -> S4
or you could have:
S4 -> S0
and add:
D0 -> D1
in both cases the source of the added move overlaps with the destination of an
existing one, but the existing parallel move won't be changed, if I am reading
this correctly.

Is there some guarantee that these cases don't happen? If yes, I think a comment
would be useful!

[Mircea Trofin, 2016-10-12 21:09:26]

https://codereview.chromium.org/2410673002/diff/100001/src/compiler/move-opti...
File src/compiler/move-optimizer.cc (right):

https://codereview.chromium.org/2410673002/diff/100001/src/compiler/move-opti...
src/compiler/move-optimizer.cc:36: void InsertOp(OperandSet* set, const
InstructionOperand& op, int* fp_reg_reps) {
would it be overkill to define InsertOp as a member of OperandSet, and hide
insert from std::set? could be done in a separate CL - benefit is
maintainability.

https://codereview.chromium.org/2410673002/diff/100001/src/compiler/move-opti...
src/compiler/move-optimizer.cc:42: bool ContainsOpOrAlias(const OperandSet& set,
const InstructionOperand& op,
same for this API

https://codereview.chromium.org/2410673002/diff/100001/src/compiler/register-...
File src/compiler/register-allocator.cc (right):

https://codereview.chromium.org/2410673002/diff/100001/src/compiler/register-...
src/compiler/register-allocator.cc:81: return 8;
here and below, don't we have a constant somewhere, like in globals.h? If not,
should we?

https://codereview.chromium.org/2410673002/diff/100001/src/compiler/register-...
src/compiler/register-allocator.cc:2855: codes =
data()->config()->allocatable_float_codes();
would it make sense to move this logic to num_registers() and
num_allocatable_registers()?

https://codereview.chromium.org/2410673002/diff/100001/src/compiler/register-...
src/compiler/register-allocator.cc:2994: codes =
data()->config()->allocatable_simd128_codes();
see factoring comment above about the num_allocatable_registers()

https://codereview.chromium.org/2410673002/diff/100001/src/compiler/register-...
src/compiler/register-allocator.cc:3053: codes =
data()->config()->allocatable_simd128_codes();
factoring comment above

https://codereview.chromium.org/2410673002/diff/100001/src/compiler/register-...
src/compiler/register-allocator.cc:3075: use_pos[cur_reg] = range->End();
this logic around next_use appears below, too. Could you factor it out?

[bbudge, 2016-10-12 23:07:09]

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

[commit-bot: I haz the power, 2016-10-12 23:07:14]

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

[bbudge, 2016-10-12 23:07:18]

https://codereview.chromium.org/2410673002/diff/100001/src/compiler/gap-resol...
File src/compiler/gap-resolver.cc (right):

https://codereview.chromium.org/2410673002/diff/100001/src/compiler/gap-resol...
src/compiler/gap-resolver.cc:203: // Ensure source is a register, to limit swap
cases.
On 2016/10/12 17:38:41, georgia.kouveli wrote:
> Isn't it still possible for both source and register to be stack slots, as in
> the old comment?

I'm not sure why I changed that, but you are correct. I restored the original
comment.

https://codereview.chromium.org/2410673002/diff/100001/src/compiler/instructi...
File src/compiler/instruction.cc (right):

https://codereview.chromium.org/2410673002/diff/100001/src/compiler/instructi...
src/compiler/instruction.cc:277: to_eliminate->push_back(curr);
On 2016/10/12 17:38:41, georgia.kouveli wrote:
> If the representation of "move" is narrower than the representation of "curr",
> shouldn't this split "curr" like in the gap resolver instead of removing it?
> 
> I'm thinking of the case where the existing parallel move contains the move:
> D1 -> D0
> and you want to add:
> S4 -> S0
> where eliminating D1->D0 and just keeping S4->S0 doesn't have the same
semantics
> as performing S4->S0 after D1->D0.
> 
> The "if" case (destination interfering with source) seems more complicated,
you
> could have:
> D1 -> D0
> and add:
> S0 -> S4
> or you could have:
> S4 -> S0
> and add:
> D0 -> D1
> in both cases the source of the added move overlaps with the destination of an
> existing one, but the existing parallel move won't be changed, if I am reading
> this correctly.
> 
> Is there some guarantee that these cases don't happen? If yes, I think a
comment
> would be useful!

I don't think splitting is needed here in either case.

If move's destination interferes with curr's destination, that means there is no
longer a double in that register as far as the register allocator is concerned.
So it doesn't matter what's in the remaining fragment (or fragments in the case
of SIMD registers.)

If move's source interferes with curr's destination (but has a different
representation), we don't want to chain those moves together. This combination
should never occur though. It implies that curr's destination is both double and
float, which is impossible.

I hope this makes sense and I'm not missing anything in my analysis. I did add
some comments to make this a little clearer.

https://codereview.chromium.org/2410673002/diff/100001/src/compiler/move-opti...
File src/compiler/move-optimizer.cc (right):

https://codereview.chromium.org/2410673002/diff/100001/src/compiler/move-opti...
src/compiler/move-optimizer.cc:36: void InsertOp(OperandSet* set, const
InstructionOperand& op, int* fp_reg_reps) {
On 2016/10/12 21:09:26, Mircea Trofin wrote:
> would it be overkill to define InsertOp as a member of OperandSet, and hide
> insert from std::set? could be done in a separate CL - benefit is
> maintainability.

OperandSet is only a typedef. We could have it inherit from std::set or create a
wrapper class, but it seems to go against the V8 philosophy (e.g. ParallelMove,
which inherits from ZoneVector, and so exposes push_back, leading to some
confusion.)

https://codereview.chromium.org/2410673002/diff/100001/src/compiler/move-opti...
src/compiler/move-optimizer.cc:42: bool ContainsOpOrAlias(const OperandSet& set,
const InstructionOperand& op,
On 2016/10/12 21:09:26, Mircea Trofin wrote:
> same for this API

Ditto

https://codereview.chromium.org/2410673002/diff/100001/src/compiler/register-...
File src/compiler/register-allocator.cc (right):

https://codereview.chromium.org/2410673002/diff/100001/src/compiler/register-...
src/compiler/register-allocator.cc:81: return 8;
On 2016/10/12 21:09:26, Mircea Trofin wrote:
> here and below, don't we have a constant somewhere, like in globals.h? If not,
> should we?

Indeed. Done.

https://codereview.chromium.org/2410673002/diff/100001/src/compiler/register-...
src/compiler/register-allocator.cc:2855: codes =
data()->config()->allocatable_float_codes();
On 2016/10/12 21:09:26, Mircea Trofin wrote:
> would it make sense to move this logic to num_registers() and
> num_allocatable_registers()?

I'd like to avoid the procedure call for non-ARM and for non-FP and kFloat64
representations. I added a GetFPRegisterSet method to extract the logic for
kFloat32 and kSimd128 though.

Done.

https://codereview.chromium.org/2410673002/diff/100001/src/compiler/register-...
src/compiler/register-allocator.cc:2994: codes =
data()->config()->allocatable_simd128_codes();
On 2016/10/12 21:09:26, Mircea Trofin wrote:
> see factoring comment above about the num_allocatable_registers()

Done.

https://codereview.chromium.org/2410673002/diff/100001/src/compiler/register-...
src/compiler/register-allocator.cc:3053: codes =
data()->config()->allocatable_simd128_codes();
On 2016/10/12 21:09:26, Mircea Trofin wrote:
> factoring comment above

Done.

https://codereview.chromium.org/2410673002/diff/100001/src/compiler/register-...
src/compiler/register-allocator.cc:3075: use_pos[cur_reg] = range->End();
On 2016/10/12 21:09:26, Mircea Trofin wrote:
> this logic around next_use appears below, too. Could you factor it out?

I added a LiveRange;:NextLifetimePositionRegisterIsBeneficial convenience
function.

Done.

[commit-bot: I haz the power, 2016-10-12 23:35:50]

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

[commit-bot: I haz the power, 2016-10-12 23:35:51]

Dry run: This issue passed the CQ dry run.

[Mircea Trofin, 2016-10-14 20:58:40]

lgtm for my side, with one comment

https://codereview.chromium.org/2410673002/diff/100001/src/compiler/move-opti...
File src/compiler/move-optimizer.cc (right):

https://codereview.chromium.org/2410673002/diff/100001/src/compiler/move-opti...
src/compiler/move-optimizer.cc:36: void InsertOp(OperandSet* set, const
InstructionOperand& op, int* fp_reg_reps) {
On 2016/10/12 23:07:17, bbudge wrote:
> On 2016/10/12 21:09:26, Mircea Trofin wrote:
> > would it be overkill to define InsertOp as a member of OperandSet, and hide
> > insert from std::set? could be done in a separate CL - benefit is
> > maintainability.
> 
> OperandSet is only a typedef. We could have it inherit from std::set or create
a
> wrapper class, but it seems to go against the V8 philosophy (e.g.
ParallelMove,
> which inherits from ZoneVector, and so exposes push_back, leading to some
> confusion.)

Mind adding a todo on the typedef to investigate doing this? I was thinking of
private inheritance or using set as a member.

[bbudge, 2016-10-15 01:28:49]

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

[commit-bot: I haz the power, 2016-10-15 01:28:51]

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

[bbudge, 2016-10-15 01:30:26]

https://codereview.chromium.org/2410673002/diff/100001/src/compiler/move-opti...
File src/compiler/move-optimizer.cc (right):

https://codereview.chromium.org/2410673002/diff/100001/src/compiler/move-opti...
src/compiler/move-optimizer.cc:36: void InsertOp(OperandSet* set, const
InstructionOperand& op, int* fp_reg_reps) {
On 2016/10/14 20:58:39, Mircea Trofin wrote:
> On 2016/10/12 23:07:17, bbudge wrote:
> > On 2016/10/12 21:09:26, Mircea Trofin wrote:
> > > would it be overkill to define InsertOp as a member of OperandSet, and
hide
> > > insert from std::set? could be done in a separate CL - benefit is
> > > maintainability.
> > 
> > OperandSet is only a typedef. We could have it inherit from std::set or
create
> a
> > wrapper class, but it seems to go against the V8 philosophy (e.g.
> ParallelMove,
> > which inherits from ZoneVector, and so exposes push_back, leading to some
> > confusion.)
> 
> Mind adding a todo on the typedef to investigate doing this? I was thinking of
> private inheritance or using set as a member.

I replaced the typedef with a class. It's much cleaner since I can encapsulate
the FP rep tracking in the class. Good suggestion!

[commit-bot: I haz the power, 2016-10-15 01:59:17]

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

[commit-bot: I haz the power, 2016-10-15 01:59:18]

Dry run: This issue passed the CQ dry run.

[bbudge, 2016-10-15 12:52:53]

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

[commit-bot: I haz the power, 2016-10-15 12:53:03]

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-10-15 13:16:49]

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

[commit-bot: I haz the power, 2016-10-15 13:16:50]

Dry run: This issue passed the CQ dry run.

[bbudge, 2016-10-15 14:20:21]

On 2016/10/15 01:30:26, bbudge wrote:
>
https://codereview.chromium.org/2410673002/diff/100001/src/compiler/move-opti...
> File src/compiler/move-optimizer.cc (right):
> 
>
https://codereview.chromium.org/2410673002/diff/100001/src/compiler/move-opti...
> src/compiler/move-optimizer.cc:36: void InsertOp(OperandSet* set, const
> InstructionOperand& op, int* fp_reg_reps) {
> On 2016/10/14 20:58:39, Mircea Trofin wrote:
> > On 2016/10/12 23:07:17, bbudge wrote:
> > > On 2016/10/12 21:09:26, Mircea Trofin wrote:
> > > > would it be overkill to define InsertOp as a member of OperandSet, and
> hide
> > > > insert from std::set? could be done in a separate CL - benefit is
> > > > maintainability.
> > > 
> > > OperandSet is only a typedef. We could have it inherit from std::set or
> create
> > a
> > > wrapper class, but it seems to go against the V8 philosophy (e.g.
> > ParallelMove,
> > > which inherits from ZoneVector, and so exposes push_back, leading to some
> > > confusion.)
> > 
> > Mind adding a todo on the typedef to investigate doing this? I was thinking
of
> > private inheritance or using set as a member.
> 
> I replaced the typedef with a class. It's much cleaner since I can encapsulate
> the FP rep tracking in the class. Good suggestion!

Also, note that I hadn't changed 'conflicting_srcs' to do aliasing checks. Now
it does.

[Mircea Trofin, 2016-10-15 23:39:47]

On 2016/10/15 14:20:21, bbudge wrote:
> On 2016/10/15 01:30:26, bbudge wrote:
> >
>
https://codereview.chromium.org/2410673002/diff/100001/src/compiler/move-opti...
> > File src/compiler/move-optimizer.cc (right):
> > 
> >
>
https://codereview.chromium.org/2410673002/diff/100001/src/compiler/move-opti...
> > src/compiler/move-optimizer.cc:36: void InsertOp(OperandSet* set, const
> > InstructionOperand& op, int* fp_reg_reps) {
> > On 2016/10/14 20:58:39, Mircea Trofin wrote:
> > > On 2016/10/12 23:07:17, bbudge wrote:
> > > > On 2016/10/12 21:09:26, Mircea Trofin wrote:
> > > > > would it be overkill to define InsertOp as a member of OperandSet, and
> > hide
> > > > > insert from std::set? could be done in a separate CL - benefit is
> > > > > maintainability.
> > > > 
> > > > OperandSet is only a typedef. We could have it inherit from std::set or
> > create
> > > a
> > > > wrapper class, but it seems to go against the V8 philosophy (e.g.
> > > ParallelMove,
> > > > which inherits from ZoneVector, and so exposes push_back, leading to
some
> > > > confusion.)
> > > 
> > > Mind adding a todo on the typedef to investigate doing this? I was
thinking
> of
> > > private inheritance or using set as a member.
> > 
> > I replaced the typedef with a class. It's much cleaner since I can
encapsulate
> > the FP rep tracking in the class. Good suggestion!
> 
> Also, note that I hadn't changed 'conflicting_srcs' to do aliasing checks. Now
> it does.

Thanks! Looks good!

[Jarin, 2016-10-18 11:44:28]

First batch of comments.

https://codereview.chromium.org/2410673002/diff/160001/src/compiler/gap-resol...
File src/compiler/gap-resolver.cc (right):

https://codereview.chromium.org/2410673002/diff/160001/src/compiler/gap-resol...
src/compiler/gap-resolver.cc:17: #define REP_BIT(rep) (1 <<
static_cast<int>(rep));
I think the semicolon at the end might be surprising for uses in the middle of
an expression (e.g. int x = REP_BIT(r) + 1;). Perhaps remove the semicolon?

https://codereview.chromium.org/2410673002/diff/160001/src/compiler/gap-resol...
src/compiler/gap-resolver.cc:32: DCHECK(!kSimpleFPAliasing);
STATIC_ASSERT?

https://codereview.chromium.org/2410673002/diff/160001/src/compiler/gap-resol...
src/compiler/gap-resolver.cc:42: dst_rep, 0, smaller_rep, &base);
I think this code makes a lot of assumptions, which I am not sure how they are
checked.

For example, it assumes that the aliases of smaller rep fully cover the bigger
rep. It also assumes that the aliases computation is uniform for all registers.


Perhaps it would be more appropriate to check that 

aliases == 1 << (static_cast<int>(dst_rep) - static_cast<int>(smaller_rep))

No?

Actually, I think it would be more robust if you said here 

int aliases = 1 << (static_cast<int>(dst_rep) - static_cast<int>(smaller_rep))

(Or maybe even 

int aliases = 1 << (ElementSizeLog2Of(dst_rep) -
ElementSizeLog2Of(smaller_rep)); )

And then on lines 50 and 58 you call GetAliases to compute src_index and
dst_index and dcheck that GetAliases returns the number of aliases computed from
element sizes.

https://codereview.chromium.org/2410673002/diff/160001/src/compiler/gap-resol...
src/compiler/gap-resolver.cc:46: const int kBaseRep =
static_cast<int>(MachineRepresentation::kFloat32);
Nit: Should not this use MachineRepresentation::kFirstFPRepresentation rather
than kFloat32?

https://codereview.chromium.org/2410673002/diff/160001/src/compiler/gap-resol...
src/compiler/gap-resolver.cc:47: int slot_size = 1 <<
(static_cast<int>(smaller_rep) - kBaseRep);
Is this supposed to mean (element_size / kPointerSize), i.e.,

(1 << ElementSizeLog2Of(smaller_rep)) / kPointerSize

If yes, perhaps we could DCHECK that for documentation purposes.

https://codereview.chromium.org/2410673002/diff/160001/src/compiler/gap-resol...
src/compiler/gap-resolver.cc:53: src_step = -slot_size;
To be honest, I was a bit confused how the indexing works here.

I was expecting

src_index = src_loc.index() + (aliases - 1) * slot_size

here because I though the big register would take up this slot + next (aliases -
1) following slots. Could you write a short comment explaining the layout on the
stack?

https://codereview.chromium.org/2410673002/diff/160001/src/compiler/gap-resol...
src/compiler/gap-resolver.cc:73: }
Overall, I am wondering about the benefit of moving one float64 as two float32 -
I would not be surprised if it goes against processor optimizations.

https://codereview.chromium.org/2410673002/diff/160001/src/compiler/gap-resol...
File src/compiler/gap-resolver.h (right):

https://codereview.chromium.org/2410673002/diff/160001/src/compiler/gap-resol...
src/compiler/gap-resolver.h:43: MachineRepresentation split_rep_;
Please, describe what this field means.

https://codereview.chromium.org/2410673002/diff/160001/src/compiler/instructi...
File src/compiler/instruction.cc (right):

https://codereview.chromium.org/2410673002/diff/160001/src/compiler/instructi...
src/compiler/instruction.cc:90: int index_lo = index_hi - (1 <<
(static_cast<int>(rep) - kBaseRep)) + 1;
Could we express this in terms of ElementSizeLog2Of, please?

E.g., 

int index_lo = index_hi - (1 << (ElementSizeLog2Of(rep) - kPointerSizeLog2)) + 1

or 

int index_lo = index_hi - (1 << ElementSizeLog2Of(rep)) / kPointerSize + 1

https://codereview.chromium.org/2410673002/diff/160001/src/compiler/register-...
File src/compiler/register-allocator.cc (right):

https://codereview.chromium.org/2410673002/diff/160001/src/compiler/register-...
src/compiler/register-allocator.cc:2078: // Preserve fixed float registers on
archs with non-simple aliasing.
Preserve -> Clobber?

[bbudge, 2016-10-19 00:30:03]

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

[commit-bot: I haz the power, 2016-10-19 00:30:11]

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

[bbudge, 2016-10-19 00:32:21]

https://codereview.chromium.org/2410673002/diff/160001/src/compiler/gap-resol...
File src/compiler/gap-resolver.cc (right):

https://codereview.chromium.org/2410673002/diff/160001/src/compiler/gap-resol...
src/compiler/gap-resolver.cc:17: #define REP_BIT(rep) (1 <<
static_cast<int>(rep));
On 2016/10/18 11:44:27, Jarin wrote:
> I think the semicolon at the end might be surprising for uses in the middle of
> an expression (e.g. int x = REP_BIT(r) + 1;). Perhaps remove the semicolon?

Whoops. Done.

https://codereview.chromium.org/2410673002/diff/160001/src/compiler/gap-resol...
src/compiler/gap-resolver.cc:32: DCHECK(!kSimpleFPAliasing);
On 2016/10/18 11:44:27, Jarin wrote:
> STATIC_ASSERT?

Wouldn't that break non-Arm builds?

https://codereview.chromium.org/2410673002/diff/160001/src/compiler/gap-resol...
src/compiler/gap-resolver.cc:42: dst_rep, 0, smaller_rep, &base);
On 2016/10/18 11:44:27, Jarin wrote:
> I think this code makes a lot of assumptions, which I am not sure how they are
> checked.
> 
> For example, it assumes that the aliases of smaller rep fully cover the bigger
> rep. It also assumes that the aliases computation is uniform for all
registers.
> 
> 
> Perhaps it would be more appropriate to check that 
> 
> aliases == 1 << (static_cast<int>(dst_rep) - static_cast<int>(smaller_rep))
> 
> No?
> 
> Actually, I think it would be more robust if you said here 
> 
> int aliases = 1 << (static_cast<int>(dst_rep) - static_cast<int>(smaller_rep))
> 
> (Or maybe even 
> 
> int aliases = 1 << (ElementSizeLog2Of(dst_rep) -
> ElementSizeLog2Of(smaller_rep)); )
> 
> And then on lines 50 and 58 you call GetAliases to compute src_index and
> dst_index and dcheck that GetAliases returns the number of aliases computed
from
> element sizes.

Yes, it's true I'm cheating by calling GetAliases for reg 0 to get the "aliasing
factor". I've changed this to a more direct computation as you've suggested and
added a DCHECK that calls GetAliases to verify that the assumption is actually
correct.

I can't use GetAliases below, because sometimes moves involving D-registers
16-31 are split, and these don't alias S-registers (according to GetAliases),
though I need to generate moves as if S-registers 32-63 actually did exist. (The
changes I make to CodeGenerator and MacroAssembler for Arm translate such
pseudo-moves into actual instructions by using VMovLow and VMovHigh.)

https://codereview.chromium.org/2410673002/diff/160001/src/compiler/gap-resol...
src/compiler/gap-resolver.cc:46: const int kBaseRep =
static_cast<int>(MachineRepresentation::kFloat32);
On 2016/10/18 11:44:27, Jarin wrote:
> Nit: Should not this use MachineRepresentation::kFirstFPRepresentation rather
> than kFloat32?

Yep, but I changed the calculation to use ElementSizeLog2Of to match
instruction.cc as you've suggested.

https://codereview.chromium.org/2410673002/diff/160001/src/compiler/gap-resol...
src/compiler/gap-resolver.cc:47: int slot_size = 1 <<
(static_cast<int>(smaller_rep) - kBaseRep);
On 2016/10/18 11:44:27, Jarin wrote:
> Is this supposed to mean (element_size / kPointerSize), i.e.,
> 
> (1 << ElementSizeLog2Of(smaller_rep)) / kPointerSize
> 
> If yes, perhaps we could DCHECK that for documentation purposes.

Changed to use ElementSiveLog2Of.

https://codereview.chromium.org/2410673002/diff/160001/src/compiler/gap-resol...
src/compiler/gap-resolver.cc:53: src_step = -slot_size;
On 2016/10/18 11:44:27, Jarin wrote:
> To be honest, I was a bit confused how the indexing works here.
> 
> I was expecting
> 
> src_index = src_loc.index() + (aliases - 1) * slot_size
> 
> here because I though the big register would take up this slot + next (aliases
-
> 1) following slots. Could you write a short comment explaining the layout on
the
> stack?

I observed that double-aligned slots on Arm are always odd. That led me to
conclude that the slot index refers to the top word, and that the lower word is
one less. The code to allocate slots seems to confirm this:

https://cs.chromium.org/chromium/src/v8/src/compiler/frame.h?rcl=0&l=134

I've added a comment to this effect (and also that this assumes little-endian.)

https://codereview.chromium.org/2410673002/diff/160001/src/compiler/gap-resol...
src/compiler/gap-resolver.cc:73: }
On 2016/10/18 11:44:27, Jarin wrote:
> Overall, I am wondering about the benefit of moving one float64 as two float32
-
> I would not be surprised if it goes against processor optimizations.

Yes, it's going to be at least 2x worse. At least the splitting is somewhat
lazy, in that we only split when a smaller FP move is blocked by a larger one.
So none of this should apply when compiling Javascript code - only wasm, or
asm.js where float operands can be created. Also, any gaps with moves of a
single representation shouldn't split moves either.

This is the only way I could find to make the GapResolver work with
"combination" aliasing as on Arm. The big problem is that when a cycle of
blocking moves is detected on aliasing architectures, simple swapping is no
longer correct for resolving it. For example, a kFloat32 swap may split the
source operand for a larger move. We'd have to track those split registers and
generate smaller moves for them anyway.

https://codereview.chromium.org/2410673002/diff/160001/src/compiler/gap-resol...
File src/compiler/gap-resolver.h (right):

https://codereview.chromium.org/2410673002/diff/160001/src/compiler/gap-resol...
src/compiler/gap-resolver.h:43: MachineRepresentation split_rep_;
On 2016/10/18 11:44:28, Jarin wrote:
> Please, describe what this field means.

Done.

https://codereview.chromium.org/2410673002/diff/160001/src/compiler/instructi...
File src/compiler/instruction.cc (right):

https://codereview.chromium.org/2410673002/diff/160001/src/compiler/instructi...
src/compiler/instruction.cc:90: int index_lo = index_hi - (1 <<
(static_cast<int>(rep) - kBaseRep)) + 1;
On 2016/10/18 11:44:28, Jarin wrote:
> Could we express this in terms of ElementSizeLog2Of, please?
> 
> E.g., 
> 
> int index_lo = index_hi - (1 << (ElementSizeLog2Of(rep) - kPointerSizeLog2)) +
1
> 
> or 
> 
> int index_lo = index_hi - (1 << ElementSizeLog2Of(rep)) / kPointerSize + 1

Done. (both places)

https://codereview.chromium.org/2410673002/diff/160001/src/compiler/instructi...
src/compiler/instruction.cc:94: return !(other_index_hi < index_lo || index_hi <
other_index_lo);
I also De-morgan'ed this boolean expression.

https://codereview.chromium.org/2410673002/diff/160001/src/compiler/register-...
File src/compiler/register-allocator.cc (right):

https://codereview.chromium.org/2410673002/diff/160001/src/compiler/register-...
src/compiler/register-allocator.cc:2078: // Preserve fixed float registers on
archs with non-simple aliasing.
On 2016/10/18 11:44:28, Jarin wrote:
> Preserve -> Clobber?

Done.

[commit-bot: I haz the power, 2016-10-19 01:31:40]

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

[commit-bot: I haz the power, 2016-10-19 01:31:41]

Dry run: Try jobs failed on following builders:
  v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/14825)
  v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
  v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
  v8_linux_chromium_gn_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_chromium_gn_rel/bu...)
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/14842)
  v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/14783)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/11068)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/26788)
  v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/16346)
  v8_win_compile_dbg on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/2...)
  v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)
  v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/16425)

[bbudge, 2016-10-19 01:39:47]

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

[commit-bot: I haz the power, 2016-10-19 01:39:55]

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-10-19 01:50:24]

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

[commit-bot: I haz the power, 2016-10-19 01:50:25]

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

[Jarin, 2016-10-19 19:23:51]

lgtm.

https://codereview.chromium.org/2410673002/diff/160001/src/compiler/gap-resol...
File src/compiler/gap-resolver.cc (right):

https://codereview.chromium.org/2410673002/diff/160001/src/compiler/gap-resol...
src/compiler/gap-resolver.cc:32: DCHECK(!kSimpleFPAliasing);
On 2016/10/19 00:32:20, bbudge wrote:
> On 2016/10/18 11:44:27, Jarin wrote:
> > STATIC_ASSERT?
> 
> Wouldn't that break non-Arm builds?

You are right, silly me.

https://codereview.chromium.org/2410673002/diff/160001/src/compiler/gap-resol...
src/compiler/gap-resolver.cc:47: int slot_size = 1 <<
(static_cast<int>(smaller_rep) - kBaseRep);
On 2016/10/19 00:32:20, bbudge wrote:
> On 2016/10/18 11:44:27, Jarin wrote:
> > Is this supposed to mean (element_size / kPointerSize), i.e.,
> > 
> > (1 << ElementSizeLog2Of(smaller_rep)) / kPointerSize
> > 
> > If yes, perhaps we could DCHECK that for documentation purposes.
> 
> Changed to use ElementSiveLog2Of.

Actually, thinking about it more, I am not sure if the stack slot moves would
would work at all on 64 bit architectures - I think there we store 32 bit values
into 64 bit slots, and there is no way to address one of the 32-bit parts of a
64 bit value. If you agree, then we should perhaps dcheck(kpointer_size == 4).

[bbudge, 2016-10-20 22:04:44]

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

[commit-bot: I haz the power, 2016-10-20 22:04:51]

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-10-20 22:14:41]

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

[commit-bot: I haz the power, 2016-10-20 22:14:42]

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

[bbudge, 2016-10-20 22:23:18]

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

[commit-bot: I haz the power, 2016-10-20 22:23:21]

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-10-20 22:30:51]

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

[commit-bot: I haz the power, 2016-10-20 22:30:52]

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

[bbudge, 2016-10-20 23:03:20]

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

[commit-bot: I haz the power, 2016-10-20 23:03:31]

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-10-20 23:12:39]

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

[commit-bot: I haz the power, 2016-10-20 23:12:40]

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

[bbudge, 2016-10-20 23:37:00]

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

[commit-bot: I haz the power, 2016-10-20 23:37:11]

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-10-20 23:43:00]

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

[commit-bot: I haz the power, 2016-10-20 23:43:02]

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

[bbudge, 2016-10-20 23:49:17]

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

[commit-bot: I haz the power, 2016-10-20 23:49:20]

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-10-20 23:57:29]

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

[commit-bot: I haz the power, 2016-10-20 23:57:31]

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

[bbudge, 2016-10-21 00:13:33]

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

[commit-bot: I haz the power, 2016-10-21 00:13:36]

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-10-21 00:19:25]

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

[commit-bot: I haz the power, 2016-10-21 00:19:27]

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

[bbudge, 2016-10-21 01:17:27]

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

[commit-bot: I haz the power, 2016-10-21 01:17:30]

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-10-21 01:21:39]

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

[commit-bot: I haz the power, 2016-10-21 01:21:39]

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

[bbudge, 2016-10-24 20:11:54]

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

[commit-bot: I haz the power, 2016-10-24 20:12:03]

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

[bbudge, 2016-10-24 20:19:44]

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

[commit-bot: I haz the power, 2016-10-24 20:19:53]

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-10-24 20:29:22]

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

[commit-bot: I haz the power, 2016-10-24 20:29:23]

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

[bbudge, 2016-10-25 00:34:20]

Patchset #24 (id:460001) has been deleted

[bbudge, 2016-10-25 00:58:23]

I've been having trouble with the Windows build. I needed to export
InstructionOperand and ParallelMove (and MoveOperands) in order to fix linker
errors in my InstructionTest that I added.

Now I'm getting errors related to zone_allocator, that its constructor is
private. Has anyone seen anything like this?

Here's the error text:

FAILED: obj/v8_base/code-generator-ia32.obj 
ninja -t msvc -e environment.x86 -- E:\b\build\slave\cache\cipd\goma/gomacc.exe
"E:\b\depot_tools\win_toolchain\vs2013_files\03a4e939cd325d6bc5216af41b92d02dda1366a6\VC\bin\amd64_x86/cl.exe"
/nologo /showIncludes /FC @obj/v8_base/code-generator-ia32.obj.rsp /c
../../src/compiler/ia32/code-generator-ia32.cc
/Foobj/v8_base/code-generator-ia32.obj /Fd"obj/v8_base_cc.pdb"
e:\b\depot_tools\win_toolchain\vs2013_files\03a4e939cd325d6bc5216af41b92d02dda1366a6\vc\include\xmemory0(801)
: error C2248: 'v8::internal::zone_allocator<T>::zone_allocator' : cannot access
private member declared in class 'v8::internal::zone_allocator<T>'
        with
        [
            T=v8::internal::compiler::MoveOperands *
        ]
        e:\b\build\slave\win\build\v8\src\zone\zone-allocator.h(65) : see
declaration of 'v8::internal::zone_allocator<T>::zone_allocator'

https://codereview.chromium.org/2410673002/diff/160001/src/compiler/gap-resol...
File src/compiler/gap-resolver.cc (right):

https://codereview.chromium.org/2410673002/diff/160001/src/compiler/gap-resol...
src/compiler/gap-resolver.cc:47: int slot_size = 1 <<
(static_cast<int>(smaller_rep) - kBaseRep);
On 2016/10/19 19:23:51, Jarin wrote:
> On 2016/10/19 00:32:20, bbudge wrote:
> > On 2016/10/18 11:44:27, Jarin wrote:
> > > Is this supposed to mean (element_size / kPointerSize), i.e.,
> > > 
> > > (1 << ElementSizeLog2Of(smaller_rep)) / kPointerSize
> > > 
> > > If yes, perhaps we could DCHECK that for documentation purposes.
> > 
> > Changed to use ElementSiveLog2Of.
> 
> Actually, thinking about it more, I am not sure if the stack slot moves would
> would work at all on 64 bit architectures - I think there we store 32 bit
values
> into 64 bit slots, and there is no way to address one of the 32-bit parts of a
> 64 bit value. If you agree, then we should perhaps dcheck(kpointer_size == 4).

Agreed. Added DCHECK.

[jochen (gone - plz use gerrit), 2016-10-25 11:56:06]

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

[jochen (gone - plz use gerrit), 2016-10-25 11:56:08]

https://codereview.chromium.org/2410673002/diff/500001/src/compiler/instructi...
File src/compiler/instruction.h (right):

https://codereview.chromium.org/2410673002/diff/500001/src/compiler/instructi...
src/compiler/instruction.h:667: class V8_EXPORT_PRIVATE MoveOperands final :
public ZoneObject {
if you export a class, you have to annotated unexported base classes, i.e., here
it's

public NON_EXPORTED_BASE(ZoneObject) (defined in src/base/compiler-specific.h)

https://codereview.chromium.org/2410673002/diff/500001/src/compiler/instructi...
src/compiler/instruction.h:726: class V8_EXPORT_PRIVATE ParallelMove final :
public ZoneVector<MoveOperands *>,
ZoneVector<T> is implemented as std::vector<T, zone_allocator<T>> or similar.
Since we haven't yet updated to MSVS 2015 (crbug.com/603131 ), the allocator
needs to have a default constructor.

If you add a constructor like zone_allocator() : zone_(nullptr) {} the error
should go away.

The allocator also needs to have a copy constructor, so maybe you'll need to
drop the "explicit" from the existing copy constructor.

[bbudge, 2016-10-25 13:46:21]

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

[commit-bot: I haz the power, 2016-10-25 13:46:28]

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

[bbudge, 2016-10-25 14:01:10]

Jaro, after all my thrashing around, I think the only changes since your last
review are 3 new exports in instruction.h and making zone_allocator's ctor
public and defining it.

https://codereview.chromium.org/2410673002/diff/500001/src/compiler/instructi...
File src/compiler/instruction.h (right):

https://codereview.chromium.org/2410673002/diff/500001/src/compiler/instructi...
src/compiler/instruction.h:667: class V8_EXPORT_PRIVATE MoveOperands final :
public ZoneObject {
On 2016/10/25 11:56:07, jochen wrote:
> if you export a class, you have to annotated unexported base classes, i.e.,
here
> it's
> 
> public NON_EXPORTED_BASE(ZoneObject) (defined in src/base/compiler-specific.h)

Done. Here and below.

https://codereview.chromium.org/2410673002/diff/500001/src/compiler/instructi...
src/compiler/instruction.h:726: class V8_EXPORT_PRIVATE ParallelMove final :
public ZoneVector<MoveOperands *>,
On 2016/10/25 11:56:07, jochen wrote:
> ZoneVector<T> is implemented as std::vector<T, zone_allocator<T>> or similar.
> Since we haven't yet updated to MSVS 2015 (crbug.com/603131 ), the allocator
> needs to have a default constructor.
> 
> If you add a constructor like zone_allocator() : zone_(nullptr) {} the error
> should go away.
> 
> The allocator also needs to have a copy constructor, so maybe you'll need to
> drop the "explicit" from the existing copy constructor.

I stumbled on that fix in patchset #22 (adding a default constructor), but it
has to be public, which seems undesirable in this case. Should I add a TODO to
remove the ctor when we update to MSVS 2015?

Thanks for your help.

[commit-bot: I haz the power, 2016-10-25 15:01:49]

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

[commit-bot: I haz the power, 2016-10-25 15:01:50]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2016-10-26 08:19:28]

On 2016/10/25 at 14:01:10, bbudge wrote:
> Jaro, after all my thrashing around, I think the only changes since your last
review are 3 new exports in instruction.h and making zone_allocator's ctor
public and defining it.
> 
>
https://codereview.chromium.org/2410673002/diff/500001/src/compiler/instructi...
> File src/compiler/instruction.h (right):
> 
>
https://codereview.chromium.org/2410673002/diff/500001/src/compiler/instructi...
> src/compiler/instruction.h:667: class V8_EXPORT_PRIVATE MoveOperands final :
public ZoneObject {
> On 2016/10/25 11:56:07, jochen wrote:
> > if you export a class, you have to annotated unexported base classes, i.e.,
here
> > it's
> > 
> > public NON_EXPORTED_BASE(ZoneObject) (defined in
src/base/compiler-specific.h)
> 
> Done. Here and below.
> 
>
https://codereview.chromium.org/2410673002/diff/500001/src/compiler/instructi...
> src/compiler/instruction.h:726: class V8_EXPORT_PRIVATE ParallelMove final :
public ZoneVector<MoveOperands *>,
> On 2016/10/25 11:56:07, jochen wrote:
> > ZoneVector<T> is implemented as std::vector<T, zone_allocator<T>> or
similar.
> > Since we haven't yet updated to MSVS 2015 (crbug.com/603131 ), the allocator
> > needs to have a default constructor.
> > 
> > If you add a constructor like zone_allocator() : zone_(nullptr) {} the error
> > should go away.
> > 
> > The allocator also needs to have a copy constructor, so maybe you'll need to
> > drop the "explicit" from the existing copy constructor.
> 
> I stumbled on that fix in patchset #22 (adding a default constructor), but it
has to be public, which seems undesirable in this case. Should I add a TODO to
remove the ctor when we update to MSVS 2015?
> 
> Thanks for your help.

yeah, that'd be great.

I think you can also put an UNREACHABLE() in that ctor, as it should never be
invoked, even on msvs

[Jarin, 2016-10-26 11:38:16]

still lgtm (assuming jochen is approving the zone_allocator change).

[jochen (gone - plz use gerrit), 2016-10-26 13:02:40]

lgtm

[bbudge, 2016-10-26 14:18:29]

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

[commit-bot: I haz the power, 2016-10-26 14:18:37]

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-10-26 14:46:08]

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

[commit-bot: I haz the power, 2016-10-26 14:46:10]

Dry run: This issue passed the CQ dry run.

[bbudge, 2016-10-26 16:02:03]

The CQ bit was checked by bbudge@chromium.org

[bbudge, 2016-10-26 16:02:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
mtrofin@chromium.org, jochen@chromium.org, jarin@chromium.org
Link to the patchset: https://codereview.chromium.org/2410673002/#ps540001
(title: "Add a TODO.")

[commit-bot: I haz the power, 2016-10-26 16:02:11]

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-10-26 16:04:17]

Committed patchset #27 (id:540001)

[commit-bot: I haz the power, 2016-11-17 22:14:21]

Description was changed from

==========
[Turbofan] Add concept of FP register aliasing on ARM 32.

- Modifies RegisterConfiguration to specify complex aliasing on ARM 32.
- Modifies RegisterAllocator to consider aliasing.
- Modifies ParallelMove::PrepareInsertAfter to handle aliasing.
- Modifies GapResolver to split wider register moves when interference
with smaller moves is detected.
- Modifies MoveOptimizer to handle aliasing.
- Adds ARM 32 macro-assembler pseudo move instructions to handle cases where
  split moves don't correspond to actual s-registers.
- Modifies CodeGenerator::AssembleMove and AssembleSwap to handle moves of
  different widths, and moves involving pseudo-s-registers.
- Adds unit tests for FP operand interference checking and PrepareInsertAfter.
- Adds more tests of FP for the move optimizer and register allocator.

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

to

==========
[Turbofan] Add concept of FP register aliasing on ARM 32.

- Modifies RegisterConfiguration to specify complex aliasing on ARM 32.
- Modifies RegisterAllocator to consider aliasing.
- Modifies ParallelMove::PrepareInsertAfter to handle aliasing.
- Modifies GapResolver to split wider register moves when interference
with smaller moves is detected.
- Modifies MoveOptimizer to handle aliasing.
- Adds ARM 32 macro-assembler pseudo move instructions to handle cases where
  split moves don't correspond to actual s-registers.
- Modifies CodeGenerator::AssembleMove and AssembleSwap to handle moves of
  different widths, and moves involving pseudo-s-registers.
- Adds unit tests for FP operand interference checking and PrepareInsertAfter.
- Adds more tests of FP for the move optimizer and register allocator.

LOG=N
BUG=v8:4124

Committed: https://crrev.com/09ab8e6ad9440d15f3083a44ca0c1ab3ae84a036
Cr-Commit-Position: refs/heads/master@{#40597}
==========

[commit-bot: I haz the power, 2016-11-17 22:14:23]

Patchset 27 (id:??) landed as
https://crrev.com/09ab8e6ad9440d15f3083a44ca0c1ab3ae84a036
Cr-Commit-Position: refs/heads/master@{#40597}