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
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-assembler-arm64-inl.h#newcode1270
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-assembler-arm64-inl.h#newcode1285
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-assembler-arm64-inl.h#newcode1289
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-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());
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.)
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.
