[v8-dev] ARM: Improve Lithium register constraints.

ARM: Improve Lithium register constraints.

  * Mandate register for input (unless there is only one use.)
  * Added macro instructions to use ip register implicitly.
  * Updated some Lithium instructions not to corrupt their inputs.
  * Reduce reliance on scratch0 and ip; further patch on this to follow.

Gives between 1% and 5% improvement on Kraken, depending on CPU.

BUG=none
TEST=none

[danno, 2013-08-19 18:50:10]

passing on to svenpanne and bmeurer, since ulan is on vacation for a few weeks.

[Benedikt Meurer, 2013-08-20 11:55:34]

https://chromiumcodereview.appspot.com/23156006/diff/1/src/globals.h
File src/globals.h (right):

https://chromiumcodereview.appspot.com/23156006/diff/1/src/globals.h#newcode170
src/globals.h:170: #define V8_BETTER_OPERAND_ON_RIGHT
I don't like this #define. Why not switch the order for the XXXMemOperand macro
instructions and use the existing logic?

[danno, 2013-08-20 14:53:20]

Again, thanks for the patch. Two more high-level comments:

- There are several, disparate changes in the CL. It would be very helpful to
break it up into several CLs with a more detailed description of each... it also
make bisection of potential problems easier in the unlikely case that there is a
bug in the code. :-)
- Your changes are pretty substantial, and you've changed register allocation
decisions based on conditional paths. However there are no added unit tests that
make sure that the new code paths are actually covered. It might be that our
existing test suite covers them all (did you verify that)? But otherwise, given
the hard-to-find and time consuming ARM bugs we've had in the last 6 months, I
would strongly encourage adding explicit coverage.

https://codereview.chromium.org/23156006/diff/1/src/arm/codegen-arm.cc
File src/arm/codegen-arm.cc (right):

https://codereview.chromium.org/23156006/diff/1/src/arm/codegen-arm.cc#newcod...
src/arm/codegen-arm.cc:777: // Register input isn't modified. All other
registers are clobbered.
nit: You already have a comment in the header about register usage, and this
one's position doesn't make it necessarily intuitive to update if things change.
I would discard this one and just keep the one in the header.

https://codereview.chromium.org/23156006/diff/1/src/arm/lithium-arm.cc
File src/arm/lithium-arm.cc (right):

https://codereview.chromium.org/23156006/diff/1/src/arm/lithium-arm.cc#newcod...
src/arm/lithium-arm.cc:486: if (value->UseCount() == 1) {
What is the concrete motivation behind this change and the one below? "Use" mean
precisely that it makes no performance difference where the live range of a
virtual is allocated, stack or register. This change changes this semantic in a
way I'm not convinced is a overall win. Take how we lazily rematerialize the
context register before runtime calls in deferred code (we don't do this in arm
yet, only on ia32, but it's something that is on our TODO list). I expect this
change artificially increases register pressure in such a situation, where what
we should do is eagerly spill the contant context register even though it's used
frequently in deferred code because the deferred code isn't called often.

I _think_ your motivation is to allocate constants into registers with a single
contiguous live range if they are used more than once (not sure). If so, there
is a better and cleaner (but harder) way to do this which involves changes in
the register allocator itself. Basically, we need to teach the register
allocator that it should by default try to allocate constants into registers
that have multiple uses, but split the constant range eagerly if register
pressure is too high and teach it how to rematerlialize those split constants
without spilling/reloading from the stack. Without that fundamental rework, I am
uncomfortable with this part of your change.

[m.m.capewell, 2013-08-21 10:37:26]

Thanks for your comments. The original patch developer is away; he'll follow up
on your queries tomorrow.

[vincent.belliard.fr, 2013-08-22 12:35:48]

It's quite difficult to split this patch. Most of the modifications are inter
dependent.

To test this patch, we run all the benchmarks to see the improvements.

We also made some test files which use a targeted lithium instruction. We were
able to check that the generated code was what we expected. This tests can be
easily included to the test suite. However, there is a lot of tests (more than
20) and we don't know if they can be useful for other CPU (if the CPU has more
registers, the tests should be rewrote).

Here one test for the SubI instruction (the aim is to use all the registers to
force stack slots usage):

// Flags: --allow-natives-syntax

function foo(a, b) {
  var c = a * b;
  var d = a * b + 1;
  var e = a * b + 2;
  var f = a * b + 3;
  var g = a * b + 4;
  var h = a * b + 5;
  var i = a * b + 6;
  var j = a * b + 7;
  var k = a * b + 8;
  var l = a * b + 9;
  var m = a * b + 10;
  var n = a * b + 11;
  var o = a * b + 12;
  var p = a * b + 13;
  var q = a * b + 14;
  var r = a * b + 15;
  var s = a * b + 16;
  var t = a * b + 17;
  var result = c - d;
  result += e - f;
  result += g - h;
  result += i - j;
  result += k - l;
  result += m - n;
  result += n - o;
  result += o - p;
  result += o - q;
  result += p - q;
  result += p - r;
  result += s - p;
  result += p - t;
  return result;
}

foo(10, 5);
var r1 = foo(10, 5);
%OptimizeFunctionOnNextCall(foo);
var r2 = foo(10, 5);

if (r1 != r2) print("found " + r2 + ", expected " + r1);

https://codereview.chromium.org/23156006/diff/1/src/arm/codegen-arm.cc
File src/arm/codegen-arm.cc (right):

https://codereview.chromium.org/23156006/diff/1/src/arm/codegen-arm.cc#newcod...
src/arm/codegen-arm.cc:777: // Register input isn't modified. All other
registers are clobbered.
I will discard it in the next release.

https://codereview.chromium.org/23156006/diff/1/src/arm/lithium-arm.cc
File src/arm/lithium-arm.cc (right):

https://codereview.chromium.org/23156006/diff/1/src/arm/lithium-arm.cc#newcod...
src/arm/lithium-arm.cc:486: if (value->UseCount() == 1) {
The register allocation has been made for IA32. The constraints with ARM are not
the same.
With ARM, the performances will never be the same with a stack slot or with a
register. The operation will always be faster with a register because, with a
stack slot, we must first load the stack slot into a register.

Our first idea was to remove the "Use" function and to always call
"UseRegister". This gave some overall improvements but, when the value was used
only once, it was sometimes a little slower because it asked the allocator to
reserve a register. The need of an extra register cause the allocator to store
some other values on the stack.

For some instructions, it's possible to load a stack slot into the IP register
(it was the meaning of the EmitLoadRegister) but it's only faster if the value
is only used once (if the value is used twice, it would be load twice and this
is slower with ARM).

I think that we don't change too much the meaning of use. The new meaning is
now:
If the value is used once, use either a register or a stack slot, else always
use a register.

It's sometimes difficult for ARM to share the IA32 code. This modification was
done to continue to share the IA32 code but with the maximum of performances for
ARM.

One last point, we removed the EmitLoadRegister function because IP should't be
used outside the macro assembler. We add some functions (XXXMemOperand) which
have the same advantage but which keep the IP use inside the macro assembler.

https://codereview.chromium.org/23156006/diff/1/src/globals.h
File src/globals.h (right):

https://codereview.chromium.org/23156006/diff/1/src/globals.h#newcode170
src/globals.h:170: #define V8_BETTER_OPERAND_ON_RIGHT
Once again, the code has been made for IA32 where operands only used once are
better at left because operations clobber the left value.
With ARM, we have 3 operands operations so there is no advantage to put used
once operands on the left.

The XXXMemOperand functions could have been defined with the MemOperand on the
left instead of on the right. However, it would have been not consistent with
other macro assembler functions.

The choice of left operand is arbitrary (but this is good for IA32). We only put
this define to be able to make the better choice for a CPU.

It was difficult to find a good name for the define (and we can't use the
ARCH_ARM define directly in hydrogen-instructions.h). If you have some
suggestions we can change the name.

[Benedikt Meurer, 2013-08-23 07:41:02]

I have had a closer look again: We really need to split this CL into smaller
pieces, otherwise we'll either have a hard time in case we break some corner
case with this change or we'll have to revert everything and you'll have to
start over again.

https://codereview.chromium.org/23156006/diff/1/src/globals.h
File src/globals.h (right):

https://codereview.chromium.org/23156006/diff/1/src/globals.h#newcode170
src/globals.h:170: #define V8_BETTER_OPERAND_ON_RIGHT
On 2013/08/22 12:35:48, vincent.belliard.fr wrote:
> Once again, the code has been made for IA32 where operands only used once are
> better at left because operations clobber the left value.
> With ARM, we have 3 operands operations so there is no advantage to put used
> once operands on the left.
> 
> The XXXMemOperand functions could have been defined with the MemOperand on the
> left instead of on the right. However, it would have been not consistent with
> other macro assembler functions.
> 
> The choice of left operand is arbitrary (but this is good for IA32). We only
put
> this define to be able to make the better choice for a CPU.
> 
> It was difficult to find a good name for the define (and we can't use the
> ARCH_ARM define directly in hydrogen-instructions.h). If you have some
> suggestions we can change the name.

I still don't see why this macro is needed at all. As you say, ARM has 3 operand
instructions, so it doesn't matter and we could as well have the better operand
on the left there as well. What am I missing?

[danno, 2013-08-24 21:04:10]

And like I said in my original review, the ARM-specific changes for handling
constants are not OK, they definitely don't look good to me. Please wait to even
consider landing this until I'm back from vacation on September 11th. I also +1
the splitting up of this CL, at first blush it looks possible.

[Rodolph Perfetta, 2013-08-28 17:42:15]

On 2013/08/24 21:04:10, danno wrote:
> And like I said in my original review, the ARM-specific changes for handling
> constants are not OK, they definitely don't look good to me. Please wait to
even
> consider landing this until I'm back from vacation on September 11th. I also
+1
> the splitting up of this CL, at first blush it looks possible.

We are going to rewrite and break down this patch. We will start by uploading
patches improving lithium instructions (not corrupting input or generating
better code sequence). These changes are not affected by the issues mentioned
above. We'll then proposed patches to address the issues discussed above.

[Rodolph Perfetta, 2013-08-28 17:50:11]

On 2013/08/23 07:41:02, Benedikt Meurer wrote:
> I have had a closer look again: We really need to split this CL into smaller
> pieces, otherwise we'll either have a hard time in case we break some corner
> case with this change or we'll have to revert everything and you'll have to
> start over again.
> 
> https://codereview.chromium.org/23156006/diff/1/src/globals.h
> File src/globals.h (right):
> 
> https://codereview.chromium.org/23156006/diff/1/src/globals.h#newcode170
> src/globals.h:170: #define V8_BETTER_OPERAND_ON_RIGHT
> On 2013/08/22 12:35:48, vincent.belliard.fr wrote:
> > Once again, the code has been made for IA32 where operands only used once
are
> > better at left because operations clobber the left value.
> > With ARM, we have 3 operands operations so there is no advantage to put used
> > once operands on the left.
> > 
> > The XXXMemOperand functions could have been defined with the MemOperand on
the
> > left instead of on the right. However, it would have been not consistent
with
> > other macro assembler functions.
> > 
> > The choice of left operand is arbitrary (but this is good for IA32). We only
> put
> > this define to be able to make the better choice for a CPU.
> > 
> > It was difficult to find a good name for the define (and we can't use the
> > ARCH_ARM define directly in hydrogen-instructions.h). If you have some
> > suggestions we can change the name.
> 
> I still don't see why this macro is needed at all. As you say, ARM has 3
operand
> instructions, so it doesn't matter and we could as well have the better
operand
> on the left there as well. What am I missing?

You are right so the better operand could be on the left. It is on the right in
this CL only because this make the instruction more "ARM like". In ARM, for
three operands instructions the second operand is always a register, the third
operand on the other hand is more flexible: it can be a small constant with
optional shit or a register with optional shift. So we decided to mimic this
behaviour but as you pointed out this could be changed. Let us know if you think
a better operand on the left would be less confusing.

[Sven Panne, 2014-06-13 11:22:57]

Some parts of this CL have already been landed in separate CLs:

   https://codereview.chromium.org/23452022
   https://codereview.chromium.org/24278004

Should the remaining parts of this CL be re-uploaded in a new (small) CL or
shall we simply close this?

Please try to split CLs into self-contained changes, so we avoid this kind of SW
archaeology in the future. :-)

[vincent.belliard, 2014-06-13 11:27:59]

> Should the remaining parts of this CL be re-uploaded in a new (small) CL or
> shall we simply close this?

As requested, this CL has been split into several small CLs. As far as I know,
everything from this CL have been implemented. You can close it.