Thanks Jacob
https://codereview.chromium.org/264773004/diff/40001/src/arm64/macro-assembler-arm64-inl.h
File src/arm64/macro-assembler-arm64-inl.h (right):
https://codereview.chromium.org/264773004/diff/40001/src/arm64/macro-assembler-arm64-inl.h#newcode1258
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-assembler-arm64-inl.h#newcode1270
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-assembler-arm64-inl.h#newcode1285
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-assembler-arm64-inl.h#newcode1289
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-assembler-arm64.cc
File src/arm64/macro-assembler-arm64.cc (left):
https://codereview.chromium.org/264773004/diff/40001/src/arm64/macro-assembler-arm64.cc#oldcode808
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?
https://codereview.chromium.org/264773004/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
