[turbofan, arm64] Fix native stack parameters on arm64.

[turbofan, arm64] Fix native stack parameters on arm64.

I added a flag to the CallDescriptor which indicates that the native
stack should be used for a CallObject instead of the js stack on arm64.

Additionally I removed the use of EmitPrepareArguments because the
current implementation does not work when float and int parameters are
mixed. I plan to fix it in a future CL, because currently I have a
problem figuring out the type of a parameter.

R=titzer@chromium.org, v8-arm-ports@googlegroups.com

Committed: https://crrev.com/282e9411f2aa71760a5983fac420fb2ec7836fca
Cr-Commit-Position: refs/heads/master@{#32577}

[jbramley, 2015-12-03 11:22:34]

jacob.bramley@arm.com changed reviewers:
+ jacob.bramley@arm.com

[jbramley, 2015-12-03 11:22:34]

https://codereview.chromium.org/1494123002/diff/1/src/compiler/arm64/code-gen...
File src/compiler/arm64/code-generator-arm64.cc (right):

https://codereview.chromium.org/1494123002/diff/1/src/compiler/arm64/code-gen...
src/compiler/arm64/code-generator-arm64.cc:890: __
PokePair(i.InputFloat64Register(1), i.InputFloat64Register(0),
Consider: DCHECK(instr->InputAt(1)->IsDoubleRegister());

Also, the reverse in the "else" case.

https://codereview.chromium.org/1494123002/diff/1/src/compiler/arm64/code-gen...
src/compiler/arm64/code-generator-arm64.cc:1313: __ SetStackPointer(csp);
Is it jssp by default? (I can't remember.) If it is jssp by default, then surely
we don't need to SetStackPointer(jssp) in the needs_frame() case above.

https://codereview.chromium.org/1494123002/diff/1/src/compiler/arm64/code-gen...
src/compiler/arm64/code-generator-arm64.cc:1335: if
(csp.Is(masm()->StackPointer()) && !descriptor->UseNativeStack()) {
Why not for the UseNativeStack() case?

In particular, if csp is left without 16-byte alignment, subsequent stack
operations will fail. (The restriction is more severe than described in the
comment.)

https://codereview.chromium.org/1494123002/diff/1/src/compiler/arm64/code-gen...
src/compiler/arm64/code-generator-arm64.cc:1406: }
Why not just "pop_count += (pop_count & 1)"?

[titzer, 2015-12-03 11:31:59]

https://codereview.chromium.org/1494123002/diff/1/src/compiler/arm64/code-gen...
File src/compiler/arm64/code-generator-arm64.cc (right):

https://codereview.chromium.org/1494123002/diff/1/src/compiler/arm64/code-gen...
src/compiler/arm64/code-generator-arm64.cc:1313: __ SetStackPointer(csp);
On 2015/12/03 11:22:34, jbramley wrote:
> Is it jssp by default? (I can't remember.) If it is jssp by default, then
surely
> we don't need to SetStackPointer(jssp) in the needs_frame() case above.

Actually, I think it's best to be explicit in both of these cases.

https://codereview.chromium.org/1494123002/diff/1/test/cctest/compiler/test-r...
File test/cctest/compiler/test-run-native-calls.cc (left):

https://codereview.chromium.org/1494123002/diff/1/test/cctest/compiler/test-r...
test/cctest/compiler/test-run-native-calls.cc:24: #if V8_TARGET_ARCH_ARM64
You can go ahead and remove the uses of this predicate altogether (yay!)

[ahaas, 2015-12-03 15:25:06]

https://codereview.chromium.org/1494123002/diff/1/src/compiler/arm64/code-gen...
File src/compiler/arm64/code-generator-arm64.cc (right):

https://codereview.chromium.org/1494123002/diff/1/src/compiler/arm64/code-gen...
src/compiler/arm64/code-generator-arm64.cc:890: __
PokePair(i.InputFloat64Register(1), i.InputFloat64Register(0),
On 2015/12/03 at 11:22:34, jbramley wrote:
> Consider: DCHECK(instr->InputAt(1)->IsDoubleRegister());
> 
> Also, the reverse in the "else" case.

I didn't add this DCHECK because i.InputFloat64Register(1) in the next line
crashes anyways if the DCHECK would fail.

https://codereview.chromium.org/1494123002/diff/1/src/compiler/arm64/code-gen...
src/compiler/arm64/code-generator-arm64.cc:1313: __ SetStackPointer(csp);
On 2015/12/03 at 11:31:59, titzer wrote:
> On 2015/12/03 11:22:34, jbramley wrote:
> > Is it jssp by default? (I can't remember.) If it is jssp by default, then
surely
> > we don't need to SetStackPointer(jssp) in the needs_frame() case above.
> 
> Actually, I think it's best to be explicit in both of these cases.

Done, I made it explicit in both cases.

https://codereview.chromium.org/1494123002/diff/1/src/compiler/arm64/code-gen...
src/compiler/arm64/code-generator-arm64.cc:1335: if
(csp.Is(masm()->StackPointer()) && !descriptor->UseNativeStack()) {
On 2015/12/03 at 11:22:34, jbramley wrote:
> Why not for the UseNativeStack() case?
> 
> In particular, if csp is left without 16-byte alignment, subsequent stack
operations will fail. (The restriction is more severe than described in the
comment.)


The additional condition is required because AlignSavedCalleeRegisterSlots sets
frame()->needs_frame() to true, which causes problems later in AssembleReturn.

I changed the condition to "if (csp.Is(masm()->StackPointer()) &&
frame()->needs_frame())" now. This is correct, because when
frame()->needs_frame() is false, then the call to AlignSavedCalleeRegisterSlots
is guaranteed to return 0.

https://codereview.chromium.org/1494123002/diff/1/src/compiler/arm64/code-gen...
src/compiler/arm64/code-generator-arm64.cc:1406: }
On 2015/12/03 at 11:22:34, jbramley wrote:
> Why not just "pop_count += (pop_count & 1)"?

Done.

https://codereview.chromium.org/1494123002/diff/1/test/cctest/compiler/test-r...
File test/cctest/compiler/test-run-native-calls.cc (left):

https://codereview.chromium.org/1494123002/diff/1/test/cctest/compiler/test-r...
test/cctest/compiler/test-run-native-calls.cc:24: #if V8_TARGET_ARCH_ARM64
On 2015/12/03 at 11:31:59, titzer wrote:
> You can go ahead and remove the uses of this predicate altogether (yay!)

Done.

[titzer, 2015-12-03 15:33:18]

On 2015/12/03 15:25:06, ahaas wrote:
>
https://codereview.chromium.org/1494123002/diff/1/src/compiler/arm64/code-gen...
> File src/compiler/arm64/code-generator-arm64.cc (right):
> 
>
https://codereview.chromium.org/1494123002/diff/1/src/compiler/arm64/code-gen...
> src/compiler/arm64/code-generator-arm64.cc:890: __
> PokePair(i.InputFloat64Register(1), i.InputFloat64Register(0),
> On 2015/12/03 at 11:22:34, jbramley wrote:
> > Consider: DCHECK(instr->InputAt(1)->IsDoubleRegister());
> > 
> > Also, the reverse in the "else" case.
> 
> I didn't add this DCHECK because i.InputFloat64Register(1) in the next line
> crashes anyways if the DCHECK would fail.
> 
>
https://codereview.chromium.org/1494123002/diff/1/src/compiler/arm64/code-gen...
> src/compiler/arm64/code-generator-arm64.cc:1313: __ SetStackPointer(csp);
> On 2015/12/03 at 11:31:59, titzer wrote:
> > On 2015/12/03 11:22:34, jbramley wrote:
> > > Is it jssp by default? (I can't remember.) If it is jssp by default, then
> surely
> > > we don't need to SetStackPointer(jssp) in the needs_frame() case above.
> > 
> > Actually, I think it's best to be explicit in both of these cases.
> 
> Done, I made it explicit in both cases.
> 
>
https://codereview.chromium.org/1494123002/diff/1/src/compiler/arm64/code-gen...
> src/compiler/arm64/code-generator-arm64.cc:1335: if
> (csp.Is(masm()->StackPointer()) && !descriptor->UseNativeStack()) {
> On 2015/12/03 at 11:22:34, jbramley wrote:
> > Why not for the UseNativeStack() case?
> > 
> > In particular, if csp is left without 16-byte alignment, subsequent stack
> operations will fail. (The restriction is more severe than described in the
> comment.)
> 
> 
> The additional condition is required because AlignSavedCalleeRegisterSlots
sets
> frame()->needs_frame() to true, which causes problems later in AssembleReturn.
> 
> I changed the condition to "if (csp.Is(masm()->StackPointer()) &&
> frame()->needs_frame())" now. This is correct, because when
> frame()->needs_frame() is false, then the call to
AlignSavedCalleeRegisterSlots
> is guaranteed to return 0.
> 
>
https://codereview.chromium.org/1494123002/diff/1/src/compiler/arm64/code-gen...
> src/compiler/arm64/code-generator-arm64.cc:1406: }
> On 2015/12/03 at 11:22:34, jbramley wrote:
> > Why not just "pop_count += (pop_count & 1)"?
> 
> Done.
> 
>
https://codereview.chromium.org/1494123002/diff/1/test/cctest/compiler/test-r...
> File test/cctest/compiler/test-run-native-calls.cc (left):
> 
>
https://codereview.chromium.org/1494123002/diff/1/test/cctest/compiler/test-r...
> test/cctest/compiler/test-run-native-calls.cc:24: #if V8_TARGET_ARCH_ARM64
> On 2015/12/03 at 11:31:59, titzer wrote:
> > You can go ahead and remove the uses of this predicate altogether (yay!)
> 
> Done.

lgtm

[ahaas, 2015-12-03 15:35:13]

The CQ bit was checked by ahaas@chromium.org

[commit-bot: I haz the power, 2015-12-03 15:35:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1494123002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1494123002/20001

[commit-bot: I haz the power, 2015-12-03 16:49:21]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-12-03 16:50:03]

Description was changed from

==========
[turbofan, arm64] Fix native stack parameters on arm64.

I added a flag to the CallDescriptor which indicates that the native
stack should be used for a CallObject instead of the js stack on arm64.

Additionally I removed the use of EmitPrepareArguments because the
current implementation does not work when float and int parameters are
mixed. I plan to fix it in a future CL, because currently I have a
problem figuring out the type of a parameter.

R=titzer@chromium.org, v8-arm-ports@googlegroups.com
==========

to

==========
[turbofan, arm64] Fix native stack parameters on arm64.

I added a flag to the CallDescriptor which indicates that the native
stack should be used for a CallObject instead of the js stack on arm64.

Additionally I removed the use of EmitPrepareArguments because the
current implementation does not work when float and int parameters are
mixed. I plan to fix it in a future CL, because currently I have a
problem figuring out the type of a parameter.

R=titzer@chromium.org, v8-arm-ports@googlegroups.com

Committed: https://crrev.com/282e9411f2aa71760a5983fac420fb2ec7836fca
Cr-Commit-Position: refs/heads/master@{#32577}
==========

[commit-bot: I haz the power, 2015-12-03 16:50:03]

Patchset 2 (id:??) landed as
https://crrev.com/282e9411f2aa71760a5983fac420fb2ec7836fca
Cr-Commit-Position: refs/heads/master@{#32577}