Arm64: Ensure that csp is always aligned to 16 byte values even if jssp is not.

Arm64: Ensure that csp is always aligned to 16 byte values even if jssp is not.

Even although the Arm64 specification specifies that csp
only needs to be aligned to 16 bytes if it is dereferenced, some implementations show poor performance if csp is every set to a non-aligned value.  This CL ensures that csp is always aligned to 16 byte values on these platforms and adds checks to ensure this in debug mode.

Also makes the following change:
 - Enable CPU support for arm64 to enable probing of cpu implementer and cpu part.
 - Add ALWAYS_ALIGN_CSP CpuFeature for Arm64 and set it based on runtime probing of the cpu implementer.
 - Rename PrepareForPush and PrepareForPop to PushPreamble and PopPostamble and move PopPostable after the pop.
 -

R=jacob.bramley@arm.com, ulan@chromium.org

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

[jbramley, 2014-05-01 15:17:10]

https://codereview.chromium.org/264773004/diff/40001/src/arm64/macro-assemble...
File src/arm64/macro-assembler-arm64-inl.h (right):

https://codereview.chromium.org/264773004/diff/40001/src/arm64/macro-assemble...
src/arm64/macro-assembler-arm64-inl.h:1258: // TODO(jbramley): Several callers
rely on this not using scratch
Since we added UseScratchRegisterScope, I don't think this is true. It used to
be important so we could do `Push(..., ip0, ...)`, but we don't do that any more
unless we first acquire ip0 (making it unavailable). It might be worth checking
that now, since it would greatly simplify this function.

https://codereview.chromium.org/264773004/diff/40001/src/arm64/macro-assemble...
src/arm64/macro-assembler-arm64-inl.h:1270: bic(csp, StackPointer(), 0xf);
This will generate two extra instructions (bic + sub) for every push, instead of
one (sub), so it will slow down the processors that don't care about the value
in csp. For that reason alone, I have to give this 'not lgtm'.

Could you do this change only on the affected platform? Can we detect it at
runtime? I know this kind of thing is ugly, but the csp-maintenance is
abstracted enough that we wouldn't have to have too many alternative code paths.

https://codereview.chromium.org/264773004/diff/40001/src/arm64/macro-assemble...
src/arm64/macro-assembler-arm64-inl.h:1285: void
MacroAssembler::SyncSystemStackPointer() {
This is never _necessary_, so it might be a good idea to
ASSERT(emit_debug_code()) just to make sure that we don't emit unnecessary code.

Also, since this is debug code, we don't need to worry about performance, and
can just Mov(csp, StackPointer()).

https://codereview.chromium.org/264773004/diff/40001/src/arm64/macro-assemble...
src/arm64/macro-assembler-arm64-inl.h:1289: sub(csp, csp, 0x10);
Why is the sub necessary? Isn't the bic enough?

https://codereview.chromium.org/264773004/diff/40001/src/arm64/macro-assemble...
File src/arm64/macro-assembler-arm64.cc (left):

https://codereview.chromium.org/264773004/diff/40001/src/arm64/macro-assemble...
src/arm64/macro-assembler-arm64.cc:808: Mov(csp, StackPointer());
It would be better to leave this here (or replace it with the new
SyncSystemStackPointer). Calling SyncSystemStackPointer in PrepareForPop works,
but it makes the check less strict than it could be. (I'd like this check to be
as strict as possible because bugs in this area would be hard to reproduce.)

[rmcilroy, 2014-05-01 18:29:11]

Thanks Jacob

https://codereview.chromium.org/264773004/diff/40001/src/arm64/macro-assemble...
File src/arm64/macro-assembler-arm64-inl.h (right):

https://codereview.chromium.org/264773004/diff/40001/src/arm64/macro-assemble...
src/arm64/macro-assembler-arm64-inl.h:1258: // TODO(jbramley): Several callers
rely on this not using scratch
On 2014/05/01 15:17:10, jbramley wrote:
> Since we added UseScratchRegisterScope, I don't think this is true. It used to
> be important so we could do `Push(..., ip0, ...)`, but we don't do that any
more
> unless we first acquire ip0 (making it unavailable). It might be worth
checking
> that now, since it would greatly simplify this function.

Yes I had hoped that was true and tried it originally, however, unfortunately we
still reach here with no remaining temp registers in
LCodeGen::PushSafepointRegistersScope.

https://codereview.chromium.org/264773004/diff/40001/src/arm64/macro-assemble...
src/arm64/macro-assembler-arm64-inl.h:1270: bic(csp, StackPointer(), 0xf);
On 2014/05/01 15:17:10, jbramley wrote:
> This will generate two extra instructions (bic + sub) for every push, instead
of
> one (sub), so it will slow down the processors that don't care about the value
> in csp. For that reason alone, I have to give this 'not lgtm'.
> 
> Could you do this change only on the affected platform? Can we detect it at
> runtime? I know this kind of thing is ugly, but the csp-maintenance is
> abstracted enough that we wouldn't have to have too many alternative code
paths.

Sure I understand your concern.  We are not yet sure whether this CL will be
necessary (it has a very big impact currently, but there may be other ways
around the issue.   I will investigate whether this can be gated on the platform
if we decide to land this.

https://codereview.chromium.org/264773004/diff/40001/src/arm64/macro-assemble...
src/arm64/macro-assembler-arm64-inl.h:1285: void
MacroAssembler::SyncSystemStackPointer() {
On 2014/05/01 15:17:10, jbramley wrote:
> This is never _necessary_, so it might be a good idea to
> ASSERT(emit_debug_code()) just to make sure that we don't emit unnecessary
code.
> 
> Also, since this is debug code, we don't need to worry about performance, and
> can just Mov(csp, StackPointer()).

Done.

https://codereview.chromium.org/264773004/diff/40001/src/arm64/macro-assemble...
src/arm64/macro-assembler-arm64-inl.h:1289: sub(csp, csp, 0x10);
On 2014/05/01 15:17:10, jbramley wrote:
> Why is the sub necessary? Isn't the bic enough?

You are right, the bic is enough (this is an artifact of a previous approach I
was using to align). Done.

https://codereview.chromium.org/264773004/diff/40001/src/arm64/macro-assemble...
File src/arm64/macro-assembler-arm64.cc (left):

https://codereview.chromium.org/264773004/diff/40001/src/arm64/macro-assemble...
src/arm64/macro-assembler-arm64.cc:808: Mov(csp, StackPointer());
On 2014/05/01 15:17:10, jbramley wrote:
> It would be better to leave this here (or replace it with the new
> SyncSystemStackPointer). Calling SyncSystemStackPointer in PrepareForPop
works,
> but it makes the check less strict than it could be. (I'd like this check to
be
> as strict as possible because bugs in this area would be hard to reproduce.)

I see what you mean, but actually, looking at it again I actually think
PrepareForPop should be called after the PopHelper - it doesn't actually prepare
anything it just asserts that the stack is in a valid state and I think this
makes more sense to do after the PopHelper has been run.  I've made a change to
call it PushPreamble and PopPostamble and moved the calls about as appropriate. 
Wdyt?

[ulan, 2014-05-02 08:08:46]

There are other places assign to the csp directly:
RegExpMacroAssemblerARM64::GetCode, MacroAssembler::LogicalMacro
Maybe add debug checks that assert that the csp is aligned after the assignment?

> I will investigate whether this can be gated on the platform if we decide to
land this.
I agree, that seems to be the right approach. It would be nice to have a flag
for this too, so that we could quickly check how unaligned csp affects
performance on a particular platform.

[jbramley, 2014-05-02 09:17:31]

On 2014/05/01 18:29:11, rmcilroy wrote:
> > Since we added UseScratchRegisterScope, I don't think this is true.
> > It used to be important so we could do `Push(..., ip0, ...)`, but we
> > don't do that any more unless we first acquire ip0 (making it
> > unavailable). It might be worth checking that now, since it would
> > greatly simplify this function.
> 
> Yes I had hoped that was true and tried it originally, however,
> unfortunately we still reach here with no remaining temp registers in
> LCodeGen::PushSafepointRegistersScope.

Ok, fair enough.

> Sure I understand your concern.  We are not yet sure whether this CL
> will be necessary (it has a very big impact currently, but there may
> be other ways around the issue.   I will investigate whether this can
> be gated on the platform if we decide to land this.

There's something else I considered when we first learned about the
stack pointer problem: We could set csp to the V8 stack limit, then
leave it there. We'd have to move it for calls into C code, and restore
it afterwards, but normal 'Push' calls could just ignore it. I didn't do
this to start with because there are some complications:
  - Can the (real) stack limit move? If so, we'd need to update csp
    accordingly. If it's only C calls that can move it, we could reload
    the stack limit on return to generated code.
  - There needs to be enough valid below the VM stack limit to allow
    signal handlers to run. I didn't know if we could guarantee this.

> I see what you mean, but actually, looking at it again I actually
> think PrepareForPop should be called after the PopHelper - it doesn't
> actually prepare anything it just asserts that the stack is in a valid
> state and I think this makes more sense to do after the PopHelper has
> been run.  I've made a change to call it PushPreamble and PopPostamble
> and moved the calls about as appropriate. Wdyt?

Yep, that makes perfect sense.

[rmcilroy, 2014-05-02 14:48:06]

On 2014/05/02 08:08:46, ulan wrote:
> There are other places assign to the csp directly:
> RegExpMacroAssemblerARM64::GetCode, MacroAssembler::LogicalMacro
> Maybe add debug checks that assert that the csp is aligned after the
assignment?

Done.

[rmcilroy, 2014-05-02 15:03:19]

On 2014/05/02 09:17:31, jbramley wrote:
> On 2014/05/01 18:29:11, rmcilroy wrote:
> > > Since we added UseScratchRegisterScope, I don't think this is true.
> > > It used to be important so we could do `Push(..., ip0, ...)`, but we
> > > don't do that any more unless we first acquire ip0 (making it
> > > unavailable). It might be worth checking that now, since it would
> > > greatly simplify this function.
> > 
> > Yes I had hoped that was true and tried it originally, however,
> > unfortunately we still reach here with no remaining temp registers in
> > LCodeGen::PushSafepointRegistersScope.
> 
> Ok, fair enough.
> 
> > Sure I understand your concern.  We are not yet sure whether this CL
> > will be necessary (it has a very big impact currently, but there may
> > be other ways around the issue.   I will investigate whether this can
> > be gated on the platform if we decide to land this.
> 
> There's something else I considered when we first learned about the
> stack pointer problem: We could set csp to the V8 stack limit, then
> leave it there. We'd have to move it for calls into C code, and restore
> it afterwards, but normal 'Push' calls could just ignore it. I didn't do
> this to start with because there are some complications:
>   - Can the (real) stack limit move? If so, we'd need to update csp
>     accordingly. If it's only C calls that can move it, we could reload
>     the stack limit on return to generated code.
>   - There needs to be enough valid below the VM stack limit to allow
>     signal handlers to run. I didn't know if we could guarantee this.

Yes I did consider this, but the signal handler issue was what made me avoid it.
 I don't think it would be safe to do this in general given that we don't know
how much space a signal handler might need, so I would prefer to do it this way
(it also gives us the benefit of ensuring we don't write below csp (modulo the
16 byte alignment buffer) when running on the simulator.

[rmcilroy, 2014-05-06 13:37:55]

Updated to make CSP alignment conditional on whether we are running on an
affected platform or not.  PTAL.

[rmcilroy, 2014-05-06 13:37:57]

Updated to make CSP alignment conditional on whether we are running on an
affected platform or not.  PTAL.

[jbramley, 2014-05-06 14:02:30]

lgtm

https://codereview.chromium.org/264773004/diff/100001/src/arm64/cpu-arm64.cc
File src/arm64/cpu-arm64.cc (right):

https://codereview.chromium.org/264773004/diff/100001/src/arm64/cpu-arm64.cc#...
src/arm64/cpu-arm64.cc:136: static_cast<uint64_t>(1) << ALWAYS_ALIGN_CSP;
Doesn't that fit on one line?

[ulan, 2014-05-06 14:06:43]

lgtm, thank you for fixing it.

[rmcilroy, 2014-05-06 15:55:11]

Thanks!

https://codereview.chromium.org/264773004/diff/100001/src/arm64/cpu-arm64.cc
File src/arm64/cpu-arm64.cc (right):

https://codereview.chromium.org/264773004/diff/100001/src/arm64/cpu-arm64.cc#...
src/arm64/cpu-arm64.cc:136: static_cast<uint64_t>(1) << ALWAYS_ALIGN_CSP;
On 2014/05/06 14:02:30, jbramley wrote:
> Doesn't that fit on one line?

Done.

[rmcilroy, 2014-05-06 15:56:27]

Committed patchset #5 manually as r21177 (presubmit successful).