LGTM with the comments addressed. Maybe some general comment on the
pitfalls of
using of ip as a temporary.
Do we have tests which creates large frames and cover all cases here?
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);
add(ip, base, ip) to have same order as the sub below? (4 times).
http://codereview.chromium.org/6311010/diff/9001/src/arm/assembler-arm.cc#newcode1873
src/arm/assembler-arm.cc:1873: ASSERT(!operand.rm().is_valid());
Assert for the supported addressing mode.
http://codereview.chromium.org/6311010/diff/9001/src/arm/assembler-arm.cc#newcode1958
src/arm/assembler-arm.cc:1958: ASSERT(!operand.rm().is_valid());
Assert for the supported addressing mode.
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#newcode609
src/arm/assembler-arm.h:609: return offset_ % 4 == 0 && offset_ < 1024
&& offset_ > -1024;
is_unit8(offset_ >= 0 ? offset_ >> 2 : (-offset_) >> 2) instead of
offset_ < 1024 && offset_ > -1024?
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.
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;
Use LCodeGen::scratch0 instead.
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;
Use LCodeGen::double_scratch0() 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
End comment with full stop.
http://codereview.chromium.org/6311010/diff/9001/src/arm/lithium-gap-resolver-arm.cc#newcode228
src/arm/lithium-gap-resolver-arm.cc:228: UNREACHABLE();
Missing indent.
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()) {
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?
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()) {
Ditto.
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() &&
Isn't this condition wrong? It should use vfp instructions if the
operands _are_ encodable.
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()) {
Don't we need to check operands for OffsetIsEncodable when using ip
here? And then have a way to spill both kSavedDoubleValueRegister and
kSavedValueRegister?
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;
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.
http://codereview.chromium.org/6311010/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
