A64: Tidy up Push and Pop TODOs.

A64: Tidy up Push and Pop TODOs.

This addresses several TODOs:
  - Push and Pop requests can be queued up so that arrays of Registers
    can be pushed efficiently, with just one PrepareForPush/Pop.
  - PushMultipleTimes now takes an Operand. This allows variable-length
    arguments arrays to be initialized, for example.
  - A NoUseRealAbortsScope has been added to Abort so that
    AssertStackConsistency can be called from PrepareForPush without
    introducing infinite recursion.

BUG=
R=ulan@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=19403

[ulan, 2014-02-17 11:58:59]

lgtm

[jbramley, 2014-02-17 12:08:15]

Committed patchset #1 manually as r19403 (presubmit successful).

[rmcilroy, 2014-02-17 12:31:05]

Some comments on Push/Pop argument order.

https://codereview.chromium.org/169533002/diff/1/src/a64/macro-assembler-a64.cc
File src/a64/macro-assembler-a64.cc (right):

https://codereview.chromium.org/169533002/diff/1/src/a64/macro-assembler-a64....
src/a64/macro-assembler-a64.cc:651: void
MacroAssembler::PushPopQueue::PopQueued() {
If I read this right, doing the following would not work:

PushPopQueue queue;
queue.Queue(x0);
queue.Queue(x1);

queue.PushQueued();

... Call function

queue.PopQueued();

Both because PushQueued() and PopQueued clear the queued_ field, and because the
arguments to Pop would need to be given in the reverse order as given to Push.
Would it be worth enabling this - it could save a load of boilerplate.  (Note: I
found it quite confusing that on arm you do Push(r0, r1), Pop(r0, r1), but on
a64 the Pop is reversed, e.g.,: Push(x0, x1), Pop(x1, x0) - could we unify this
to be the same as Arm?)

https://codereview.chromium.org/169533002/diff/1/src/a64/macro-assembler-a64.h
File src/a64/macro-assembler-a64.h (right):

https://codereview.chromium.org/169533002/diff/1/src/a64/macro-assembler-a64....
src/a64/macro-assembler-a64.h:2040: void PrepareForPush(int count, int size) {
PrepareForPush(count * size); }
nit - restore the comment for these methods that:
// Note that size is per register, and is specified in bytes.

[jbramley, 2014-02-17 12:43:20]

https://codereview.chromium.org/169533002/diff/1/src/a64/macro-assembler-a64.cc
File src/a64/macro-assembler-a64.cc (right):

https://codereview.chromium.org/169533002/diff/1/src/a64/macro-assembler-a64....
src/a64/macro-assembler-a64.cc:651: void
MacroAssembler::PushPopQueue::PopQueued() {
On 2014/02/17 12:31:06, rmcilroy wrote:
> If I read this right, doing the following would not work:
> 
> PushPopQueue queue;
> queue.Queue(x0);
> queue.Queue(x1);
> 
> queue.PushQueued();
> 
> ... Call function
> 
> queue.PopQueued();

That's correct. At the moment, PopQueued() isn't used (other than in the tests),
but I implemented it for completeness anyway.

> Both because PushQueued() and PopQueued clear the queued_ field, and because
the
> arguments to Pop would need to be given in the reverse order as given to Push.
> Would it be worth enabling this - it could save a load of boilerplate.  (Note:
I
> found it quite confusing that on arm you do Push(r0, r1), Pop(r0, r1), but on
> a64 the Pop is reversed, e.g.,: Push(x0, x1), Pop(x1, x0) - could we unify
this
> to be the same as Arm?)

We made a conscious decision to do things that way. On ARM, the registers are
pushed such that the lowest-indexed register gets the lowest address. Pop(r0,
r1) and Pop(r1, r0) do the same thing (though there may be a warning in V8). On
A64, that isn't the case, and we wanted the following sequences to be
equivalent:

Push(xa);
Push(xb);

Push(xa, xb);

That is, Push and Pop can be split into several calls, but the order of
operations always agrees with reading order. With this scheme, Pop has to work
in the opposite order. It would be easy to make PopQueued() reverse the list
automatically, but it would then operate differently from Pop(), as you noted.

Does that argument make sense? We spent some time thinking about it when we
started the port but that doesn't mean we can't change things.

[rmcilroy, 2014-02-17 13:45:46]

> We made a conscious decision to do things that way. On ARM, the registers are
> pushed such that the lowest-indexed register gets the lowest address. Pop(r0,
> r1) and Pop(r1, r0) do the same thing (though there may be a warning in V8). 

I don't think this is correct.  On Arm Pop always pops the rightmost register
first (from lower address), so Pop(r0, r1) and Pop(r1, r0) are different (but
only Pop(r0, r1) can use the more efficient ldm instruction).

> On A64, that isn't the case, and we wanted the following sequences to be
> equivalent:
> 
> Push(xa);
> Push(xb);
> 
> Push(xa, xb);
> 
> That is, Push and Pop can be split into several calls, but the order of
> operations always agrees with reading order. With this scheme, Pop has to work
> in the opposite order. It would be easy to make PopQueued() reverse the list
> automatically, but it would then operate differently from Pop(), as you noted.
> 
> Does that argument make sense? We spent some time thinking about it when we
> started the port but that doesn't mean we can't change things.

I can see the argument, but personally I think having Push and Pop be
symmetrical (in terms of the arguments to a Pop() being the same as the matching
call to Push()) is more readable and less-error prone than the other way around
(to check whether the push/pop is matching you only have to compare the set of
arguments, not reverse them).  I also think it is much more important to be
consistent across the architectures if possible - so if we do decided to move to
this model, we should change arm, mips and any other arches which do
multi-pop/push to be the same.

Do any other V8 team members have an opinion?

[m.m.capewell, 2014-02-17 14:03:01]

On 2014/02/17 13:45:46, rmcilroy wrote:
> Do any other V8 team members have an opinion?

Only what Jacob's already stated. We decided this early in the A64 port, as we'd
frequently want to merge Pops, but the merging often brought bugs due to Pop(a);
Pop(b) != Pop(a, b).

The only other reason I have is in a sequence such as:
Pop(a, b)
Pop(c, d)

The current scheme pops a, b, c then d. The old scheme pops b, a, d then c,
which I think is more confusing; it obeys reading order, but only in one
direction.

[ulan, 2014-02-17 14:13:10]

> Do any other V8 team members have an opinion?

I don't have strong opinion on this, but I find A64 version of Pop a bit easier
to read.

In the following example in A64, the popping order is the same as the "reading
order":
Pop(a);
Pop(b, c);
Pop(d);

So it is easy to see that we pop a, then b, then c, then d.

In ARM, this example would be:
Pop(a);
Pop(c, b);
Pop(d);

So we need to read top-to-down, right-to-left.

I agree that Push/Pop on ARM and A64 should agree on order of arguments,
otherwise it is confusing.

[rmcilroy, 2014-02-17 14:23:48]

On 2014/02/17 14:13:10, ulan wrote:
> > Do any other V8 team members have an opinion?
> 
> I don't have strong opinion on this, but I find A64 version of Pop a bit
easier
> to read.
> 
> In the following example in A64, the popping order is the same as the "reading
> order":
> Pop(a);
> Pop(b, c);
> Pop(d);
> 
> So it is easy to see that we pop a, then b, then c, then d.
> 
> In ARM, this example would be:
> Pop(a);
> Pop(c, b);
> Pop(d);
> 
> So we need to read top-to-down, right-to-left.
> 
> I agree that Push/Pop on ARM and A64 should agree on order of arguments,
> otherwise it is confusing.

Ok if the consensus is that this ordering is easier to read I'm fine with that,
but I definitely think we should move Arm across to the same model to avoid
confusion.

[ulan, 2014-02-17 16:13:08]

This broke many debug tests:
#
# Fatal error in ../src/a64/assembler-a64.cc, line 2239
# external code buffer is too small
#

Looks like because of the abort message, we are overflowing the code buffer in
Builtins::SetUp:
union { int force_alignment; byte buffer[16*KB]; } u;

[jbramley, 2014-02-17 16:16:01]

On 2014/02/17 16:13:08, ulan wrote:
> This broke many debug tests:
> #
> # Fatal error in ../src/a64/assembler-a64.cc, line 2239
> # external code buffer is too small
> #
> 
> Looks like because of the abort message, we are overflowing the code buffer in
> Builtins::SetUp:
> union { int force_alignment; byte buffer[16*KB]; } u;

Ok, that makes sense. Can we make that buffer bigger, or do we have to work
within the limit?

Our Aborts are bigger than they need to be at the moment because we encode the
whole string rather than just the BailoutReason. (The other architectures work
the same; only Throw optimises this to encode just the BailoutReason.) If we fix
that, the code size might reduce enough anyway.

[ulan, 2014-02-17 16:29:49]

> Our Aborts are bigger than they need to be at the moment because we encode the
> whole string rather than just the BailoutReason. (The other architectures work
> the same; only Throw optimises this to encode just the BailoutReason.) If we
fix
> that, the code size might reduce enough anyway.
Fixing aborts to encode just the enum seems to me a better solution than
increasing the buffer.

[jbramley, 2014-02-17 16:32:41]

On 2014/02/17 16:29:49, ulan wrote:
> > Our Aborts are bigger than they need to be at the moment because we encode
the
> > whole string rather than just the BailoutReason. (The other architectures
work
> > the same; only Throw optimises this to encode just the BailoutReason.) If we
> fix
> > that, the code size might reduce enough anyway.
> Fixing aborts to encode just the enum seems to me a better solution than
> increasing the buffer.

It does, but there's no guarantee that it will be enough. In any case, I'll work
on that and then try the push-pop fixes afterwards.

[jbramley, 2014-02-18 15:32:21]

I had to modify MakeConvertDToIFuncTrampoline to use PushPopQueue otherwise its
set of Push calls generated too much code.

[jbramley, 2014-02-18 15:33:52]

On 2014/02/18 15:32:21, jbramley wrote:
> I had to modify MakeConvertDToIFuncTrampoline to use PushPopQueue otherwise
its
> set of Push calls generated too much code.

By the way, was updating this issue the right thing to do in this situation, or
should I have created a new one?

[ulan, 2014-02-18 15:39:24]

On 2014/02/18 15:33:52, jbramley wrote:
> On 2014/02/18 15:32:21, jbramley wrote:
> > I had to modify MakeConvertDToIFuncTrampoline to use PushPopQueue otherwise
> its
> > set of Push calls generated too much code.
> 
> By the way, was updating this issue the right thing to do in this situation,
or
> should I have created a new one?

It is better to create a new one because this issue is already closed. Could you
please reupload in a new issue?

[jbramley, 2014-02-18 15:40:36]

On 2014/02/18 15:39:24, ulan wrote:
> On 2014/02/18 15:33:52, jbramley wrote:
> > On 2014/02/18 15:32:21, jbramley wrote:
> > > I had to modify MakeConvertDToIFuncTrampoline to use PushPopQueue
otherwise
> > its
> > > set of Push calls generated too much code.
> > 
> > By the way, was updating this issue the right thing to do in this situation,
> or
> > should I have created a new one?
> 
> It is better to create a new one because this issue is already closed. Could
you
> please reupload in a new issue?

Yep, no problem.

[rmcilroy, 2014-02-18 15:50:33]

On 2014/02/18 15:40:36, jbramley wrote:
> On 2014/02/18 15:39:24, ulan wrote:
> > On 2014/02/18 15:33:52, jbramley wrote:
> > > On 2014/02/18 15:32:21, jbramley wrote:
> > > > I had to modify MakeConvertDToIFuncTrampoline to use PushPopQueue
> otherwise
> > > its
> > > > set of Push calls generated too much code.
> > > 
> > > By the way, was updating this issue the right thing to do in this
situation,
> > or
> > > should I have created a new one?
> > 
> > It is better to create a new one because this issue is already closed. Could
> you
> > please reupload in a new issue?
> 
> Yep, no problem.

For future reference, a handy thing to do in this situation is to upload "patch
set 1" with exactly the same patch set as the reverted CL, then re-upload your
new CL as "patch set 2".  That way reviewers can easily compare "patch set 2"
with "patch set 1" to see only the differences you've made to reland the CL.

[jbramley, 2014-02-18 15:53:40]

On 2014/02/18 15:50:33, rmcilroy wrote:
> On 2014/02/18 15:40:36, jbramley wrote:
> > On 2014/02/18 15:39:24, ulan wrote:
> > > On 2014/02/18 15:33:52, jbramley wrote:
> > > > On 2014/02/18 15:32:21, jbramley wrote:
> > > > > I had to modify MakeConvertDToIFuncTrampoline to use PushPopQueue
> > otherwise
> > > > its
> > > > > set of Push calls generated too much code.
> > > > 
> > > > By the way, was updating this issue the right thing to do in this
> situation,
> > > or
> > > > should I have created a new one?
> > > 
> > > It is better to create a new one because this issue is already closed.
Could
> > you
> > > please reupload in a new issue?
> > 
> > Yep, no problem.
> 
> For future reference, a handy thing to do in this situation is to upload
"patch
> set 1" with exactly the same patch set as the reverted CL, then re-upload your
> new CL as "patch set 2".  That way reviewers can easily compare "patch set 2"
> with "patch set 1" to see only the differences you've made to reland the CL.

Ok, I'll try to remember that next time.

The new changes are here: https://codereview.chromium.org/170623002/

The only difference (other than an automatic rebase) is the change in
test-code-stubs-a64.cc.