Much simplified version. I would assert that { 9 } is cgen_->scratch0(),
but
scratch0() is private. I think we can change LCodeGen's scratch registers
to be
more like these. I think it is much more efficient.
Right now, it is crashing in the stack overflow test, because it is trying
to
write to sp - 4, probably in my generated code. But that sounds reasonable
to
me - is there any slack in the stack limit test?
There is also a failure in the tick-processor test, because something
doesn't
agree exactly in the output log.
http://codereview.chromium.org/6311010/diff/9001/src/arm/assembler-arm.cc
File src/arm/assembler-arm.cc (right):
http://codereview.chromium.org/6311010/diff/9001/src/arm/assembler-arm.cc#newcode1861
src/arm/assembler-arm.cc:1861: add(ip, ip, base);
On 2011/01/25 08:33:20, Søren Gjesse wrote:
add(ip, base, ip) to have same order as the sub below? (4 times).
Done.
http://codereview.chromium.org/6311010/diff/9001/src/arm/assembler-arm.cc#newcode1873
src/arm/assembler-arm.cc:1873: ASSERT(!operand.rm().is_valid());
On 2011/01/25 08:33:20, Søren Gjesse wrote:
Assert for the supported addressing mode.
Done.
http://codereview.chromium.org/6311010/diff/9001/src/arm/assembler-arm.cc#newcode1958
src/arm/assembler-arm.cc:1958: ASSERT(!operand.rm().is_valid());
On 2011/01/25 08:33:20, Søren Gjesse wrote:
Assert for the supported addressing mode.
Done.
http://codereview.chromium.org/6311010/diff/9001/src/arm/assembler-arm.h
File src/arm/assembler-arm.h (right):
http://codereview.chromium.org/6311010/diff/9001/src/arm/assembler-arm.h#newcode1077
src/arm/assembler-arm.h:1077: const MemOperand& src, // Source cannot
be a FieldOperand.
Comments are removed. These functions now work with any offsets
whatsoever.
On 2011/01/25 08:33:20, Søren Gjesse wrote:
I think the comments "Offset must be a multiple of 4" and "Source
cannot be a
FieldOperand" should be removed and a comment added before all the
vldr/vstr
functions saying that the offset must be a multiple of 4. FieldOperand
is a
macro assembler function and should not be mentioned here. Maybe also
mention
that if a MemOperand is used only addressing mode offset is aupported.
http://codereview.chromium.org/6311010/diff/9001/src/arm/lithium-gap-resolver-arm.cc
File src/arm/lithium-gap-resolver-arm.cc (right):
http://codereview.chromium.org/6311010/diff/9001/src/arm/lithium-gap-resolver-arm.cc#newcode34
src/arm/lithium-gap-resolver-arm.cc:34: static const Register
kSavedValueRegister = r9;
On 2011/01/25 08:33:20, Søren Gjesse wrote:
Use LCodeGen::scratch0 instead.
Used { 9 } instead, with an assert that it equals scratch0().
http://codereview.chromium.org/6311010/diff/9001/src/arm/lithium-gap-resolver-arm.cc#newcode35
src/arm/lithium-gap-resolver-arm.cc:35: static const DoubleRegister
kSavedDoubleValueRegister = d0;
On 2011/01/25 08:33:20, Søren Gjesse wrote:
Use LCodeGen::double_scratch0() instead.
Used { 0 } instead.
http://codereview.chromium.org/6311010/diff/9001/src/arm/lithium-gap-resolver-arm.cc#newcode198
src/arm/lithium-gap-resolver-arm.cc:198: // Restore from stack
On 2011/01/25 08:33:20, Søren Gjesse wrote:
End comment with full stop.
Done.
http://codereview.chromium.org/6311010/diff/9001/src/arm/lithium-gap-resolver-arm.cc#newcode228
src/arm/lithium-gap-resolver-arm.cc:228: UNREACHABLE();
On 2011/01/25 08:33:20, Søren Gjesse wrote:
Missing indent.
Done.
http://codereview.chromium.org/6311010/diff/9001/src/arm/lithium-gap-resolver-arm.cc#newcode262
src/arm/lithium-gap-resolver-arm.cc:262: if
(!destination_operand.OffsetIsEncodable()) {
On 2011/01/25 08:33:20, Søren Gjesse wrote:
Please explain this check for OffsetIsEncodable. This is based on the
fact that
for encodable MemOperands ip will not be used by the ldr/str, so we
don't have
to spill kSavedValueRegister.
Should the check not also include source_operand?
Done.
http://codereview.chromium.org/6311010/diff/9001/src/arm/lithium-gap-resolver-arm.cc#newcode285
src/arm/lithium-gap-resolver-arm.cc:285: if
(!destination_operand.OffsetIsEncodable()) {
On 2011/01/25 08:33:20, Søren Gjesse wrote:
Ditto.
Done.
http://codereview.chromium.org/6311010/diff/9001/src/arm/lithium-gap-resolver-arm.cc#newcode285
src/arm/lithium-gap-resolver-arm.cc:285: if
(!destination_operand.OffsetIsEncodable()) {
On 2011/01/25 08:33:20, Søren Gjesse wrote:
Ditto.
Done.
http://codereview.chromium.org/6311010/diff/9001/src/arm/lithium-gap-resolver-arm.cc#newcode322
src/arm/lithium-gap-resolver-arm.cc:322: if
(destination_operand.OffsetIsVFPEncodable() &&
On 2011/01/25 08:33:20, Søren Gjesse wrote:
Isn't this condition wrong? It should use vfp instructions if the
operands _are_
encodable.
No, we can use vfp instructions in either case. For the simple copy, we
need a temporary vfp register, which we don't always have. Using ip is
easier, in the case that we don't have a temporary vfp register. But we
can't use ip if the store will overwrite it.
http://codereview.chromium.org/6311010/diff/9001/src/arm/lithium-gap-resolver-arm.cc#newcode323
src/arm/lithium-gap-resolver-arm.cc:323:
destination_high_operand.OffsetIsVFPEncodable()) {
On 2011/01/25 08:33:20, Søren Gjesse wrote:
Don't we need to check operands for OffsetIsEncodable when using ip
here? And
then have a way to spill both kSavedDoubleValueRegister and
kSavedValueRegister?
You are right. All this is now rewritten to eliminate spilling of
registers to stack.
http://codereview.chromium.org/6311010/diff/9001/src/arm/lithium-gap-resolver-arm.cc#newcode329
src/arm/lithium-gap-resolver-arm.cc:329: cycle_breaker_spilled_ = true;
On 2011/01/25 08:33:20, Søren Gjesse wrote:
I don't know whether these three VFP load/stores touching 6 words of
memory are
actually faster than the sequence above which is only touching 4 words
of
memory.
These are done if we can't use ip. But I see we can use
kSavedValueRegister, so this is removed.
http://codereview.chromium.org/6311010/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
