ARM64: push/pop registers in stubs for safepoints

ARM64: push/pop registers in stubs for safepoints

BUG=

R=ulan@chromium.org, bmeurer@chromium.org

[ulan, 2014-03-24 11:09:25]

https://codereview.chromium.org/209933003/diff/1/src/arm64/lithium-codegen-ar...
File src/arm64/lithium-codegen-arm64.h (right):

https://codereview.chromium.org/209933003/diff/1/src/arm64/lithium-codegen-ar...
src/arm64/lithium-codegen-arm64.h:385: codegen_->masm_->Mov(to_be_pushed_lr,
lr);
to_be_pushed_lr is ip0, how do we guarantee that this doesn't get clobbered from
this point until the StoreRegistersStateStub gets control?

[vincent.belliard, 2014-03-24 11:16:59]

https://codereview.chromium.org/209933003/diff/1/src/arm64/lithium-codegen-ar...
File src/arm64/lithium-codegen-arm64.h (right):

https://codereview.chromium.org/209933003/diff/1/src/arm64/lithium-codegen-ar...
src/arm64/lithium-codegen-arm64.h:385: codegen_->masm_->Mov(to_be_pushed_lr,
lr);
On 2014/03/24 11:09:25, ulan wrote:
> to_be_pushed_lr is ip0, how do we guarantee that this doesn't get clobbered
from
> this point until the StoreRegistersStateStub gets control?

This register is reserved by the UseScratchRegisterScope so it cannot be used by
the call (which will use the next register: ip1).

After the call, we jump directly to the stub code (which has its own context)
which starts also by a UseScratchRegisterScope which also reserves
to_be_pushed_lr.

[ulan, 2014-03-24 11:31:25]

https://codereview.chromium.org/209933003/diff/1/src/arm64/lithium-codegen-ar...
File src/arm64/lithium-codegen-arm64.h (right):

https://codereview.chromium.org/209933003/diff/1/src/arm64/lithium-codegen-ar...
src/arm64/lithium-codegen-arm64.h:385: codegen_->masm_->Mov(to_be_pushed_lr,
lr);
On 2014/03/24 11:16:59, vincent.belliard.arm wrote:
> On 2014/03/24 11:09:25, ulan wrote:
> > to_be_pushed_lr is ip0, how do we guarantee that this doesn't get clobbered
> from
> > this point until the StoreRegistersStateStub gets control?
> 
> This register is reserved by the UseScratchRegisterScope so it cannot be used
by
> the call (which will use the next register: ip1).
> 
> After the call, we jump directly to the stub code (which has its own context)
> which starts also by a UseScratchRegisterScope which also reserves
> to_be_pushed_lr.
Right, and what if the ip0 is not available in the current scope because it was
already reserved before PushSafepointRegistersScope?

[vincent.belliard, 2014-03-24 11:43:28]

https://codereview.chromium.org/209933003/diff/1/src/arm64/lithium-codegen-ar...
File src/arm64/lithium-codegen-arm64.h (right):

https://codereview.chromium.org/209933003/diff/1/src/arm64/lithium-codegen-ar...
src/arm64/lithium-codegen-arm64.h:385: codegen_->masm_->Mov(to_be_pushed_lr,
lr);
On 2014/03/24 11:31:26, ulan wrote:
> On 2014/03/24 11:16:59, vincent.belliard.arm wrote:
> > On 2014/03/24 11:09:25, ulan wrote:
> > > to_be_pushed_lr is ip0, how do we guarantee that this doesn't get
clobbered
> > from
> > > this point until the StoreRegistersStateStub gets control?
> > 
> > This register is reserved by the UseScratchRegisterScope so it cannot be
used
> by
> > the call (which will use the next register: ip1).
> > 
> > After the call, we jump directly to the stub code (which has its own
context)
> > which starts also by a UseScratchRegisterScope which also reserves
> > to_be_pushed_lr.
> Right, and what if the ip0 is not available in the current scope because it
was
> already reserved before PushSafepointRegistersScope?

In debug, Acquire checks that the register is available. If each instruction
which use a PushSafepointRegistersScope is used by the at least one test (I hope
all are used) and if the tests are run in debug, it will ensure that the
register will be available.

In release, the register could be unavailable and still acquired (no test is
done during acquire). If it's not available, we will clobber it. However, it
doesn't matter because as we enter PushSafepointRegistersScope, only the
safepoint registers must be saved and ip0, ip1 can be cloberred.

[ulan, 2014-03-24 12:23:50]

https://codereview.chromium.org/209933003/diff/1/src/arm64/lithium-codegen-ar...
File src/arm64/lithium-codegen-arm64.h (right):

https://codereview.chromium.org/209933003/diff/1/src/arm64/lithium-codegen-ar...
src/arm64/lithium-codegen-arm64.h:385: codegen_->masm_->Mov(to_be_pushed_lr,
lr);
On 2014/03/24 11:43:28, vincent.belliard.arm wrote:
> On 2014/03/24 11:31:26, ulan wrote:
> > On 2014/03/24 11:16:59, vincent.belliard.arm wrote:
> > > On 2014/03/24 11:09:25, ulan wrote:
> > > > to_be_pushed_lr is ip0, how do we guarantee that this doesn't get
> clobbered
> > > from
> > > > this point until the StoreRegistersStateStub gets control?
> > > 
> > > This register is reserved by the UseScratchRegisterScope so it cannot be
> used
> > by
> > > the call (which will use the next register: ip1).
> > > 
> > > After the call, we jump directly to the stub code (which has its own
> context)
> > > which starts also by a UseScratchRegisterScope which also reserves
> > > to_be_pushed_lr.
> > Right, and what if the ip0 is not available in the current scope because it
> was
> > already reserved before PushSafepointRegistersScope?
> 
> In debug, Acquire checks that the register is available. If each instruction
> which use a PushSafepointRegistersScope is used by the at least one test (I
hope
> all are used) and if the tests are run in debug, it will ensure that the
> register will be available.
> 
> In release, the register could be unavailable and still acquired (no test is
> done during acquire). If it's not available, we will clobber it. However, it
> doesn't matter because as we enter PushSafepointRegistersScope, only the
> safepoint registers must be saved and ip0, ip1 can be cloberred.
OK, thank you for explanation. I went through the call sites of
PushSafepointRegistersScope, and they seem safe to me. Since Acquire(reg) is
unsafe in general case, let's rename it to UnsafeAcquire and add a comment there
explaining when one can use it.

https://codereview.chromium.org/209933003/diff/1/src/arm64/macro-assembler-ar...
File src/arm64/macro-assembler-arm64.h (right):

https://codereview.chromium.org/209933003/diff/1/src/arm64/macro-assembler-ar...
src/arm64/macro-assembler-arm64.h:2218: Register Acquire(const Register& reg) {
Please rename to UnsafeAcquire or something similar.

https://codereview.chromium.org/209933003/diff/1/src/arm64/macro-assembler-ar...
src/arm64/macro-assembler-arm64.h:2222: return AcquireFixed(availablefp_, reg);
It is not used and unsafe, please remove.

[vincent.belliard, 2014-03-24 16:03:52]

https://codereview.chromium.org/209933003/diff/1/src/arm64/lithium-codegen-ar...
File src/arm64/lithium-codegen-arm64.h (right):

https://codereview.chromium.org/209933003/diff/1/src/arm64/lithium-codegen-ar...
src/arm64/lithium-codegen-arm64.h:385: codegen_->masm_->Mov(to_be_pushed_lr,
lr);
On 2014/03/24 12:23:51, ulan wrote:
> On 2014/03/24 11:43:28, vincent.belliard.arm wrote:
> > On 2014/03/24 11:31:26, ulan wrote:
> > > On 2014/03/24 11:16:59, vincent.belliard.arm wrote:
> > > > On 2014/03/24 11:09:25, ulan wrote:
> > > > > to_be_pushed_lr is ip0, how do we guarantee that this doesn't get
> > clobbered
> > > > from
> > > > > this point until the StoreRegistersStateStub gets control?
> > > > 
> > > > This register is reserved by the UseScratchRegisterScope so it cannot be
> > used
> > > by
> > > > the call (which will use the next register: ip1).
> > > > 
> > > > After the call, we jump directly to the stub code (which has its own
> > context)
> > > > which starts also by a UseScratchRegisterScope which also reserves
> > > > to_be_pushed_lr.
> > > Right, and what if the ip0 is not available in the current scope because
it
> > was
> > > already reserved before PushSafepointRegistersScope?
> > 
> > In debug, Acquire checks that the register is available. If each instruction
> > which use a PushSafepointRegistersScope is used by the at least one test (I
> hope
> > all are used) and if the tests are run in debug, it will ensure that the
> > register will be available.
> > 
> > In release, the register could be unavailable and still acquired (no test is
> > done during acquire). If it's not available, we will clobber it. However, it
> > doesn't matter because as we enter PushSafepointRegistersScope, only the
> > safepoint registers must be saved and ip0, ip1 can be cloberred.
> OK, thank you for explanation. I went through the call sites of
> PushSafepointRegistersScope, and they seem safe to me. Since Acquire(reg) is
> unsafe in general case, let's rename it to UnsafeAcquire and add a comment
there
> explaining when one can use it.

done

https://codereview.chromium.org/209933003/diff/1/src/arm64/macro-assembler-ar...
File src/arm64/macro-assembler-arm64.h (right):

https://codereview.chromium.org/209933003/diff/1/src/arm64/macro-assembler-ar...
src/arm64/macro-assembler-arm64.h:2218: Register Acquire(const Register& reg) {
On 2014/03/24 12:23:51, ulan wrote:
> Please rename to UnsafeAcquire or something similar.

done

https://codereview.chromium.org/209933003/diff/1/src/arm64/macro-assembler-ar...
src/arm64/macro-assembler-arm64.h:2222: return AcquireFixed(availablefp_, reg);
On 2014/03/24 12:23:51, ulan wrote:
> It is not used and unsafe, please remove.

done

[ulan, 2014-03-24 16:19:03]

lgtm