ARM: Port new version of ParallelMove's GapResolver to ARM. Add MemOperand s...

ARM: Port new version of ParallelMove's GapResolver to ARM.  Fix error in vstr(DoubleRegister, MemOperand), where it was implemented as a vldr, in r6830.

Committed: http://code.google.com/p/v8/source/detail?r=6839

[William Hesse, 2011-01-21 14:45:58]

Not quite finished.  Some lint errors also remain.

[William Hesse, 2011-01-24 13:25:27]

Done!

[Søren Thygesen Gjesse, 2011-01-25 08:33:20]

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#new...
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#new...
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#new...
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#newc...
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#newc...
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...
File src/arm/lithium-gap-resolver-arm.cc (right):

http://codereview.chromium.org/6311010/diff/9001/src/arm/lithium-gap-resolver...
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...
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...
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...
src/arm/lithium-gap-resolver-arm.cc:228: UNREACHABLE();
Missing indent.

http://codereview.chromium.org/6311010/diff/9001/src/arm/lithium-gap-resolver...
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...
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...
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...
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...
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.

[William Hesse, 2011-01-25 17:25:12]

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#new...
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#new...
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#new...
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#newc...
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...
File src/arm/lithium-gap-resolver-arm.cc (right):

http://codereview.chromium.org/6311010/diff/9001/src/arm/lithium-gap-resolver...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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.

[Søren Thygesen Gjesse, 2011-01-26 08:10:26]

LGTM

Regarding the failing test you say "it is trying to write to sp - 4". Do you
mean sp here? If you do then something is off, as we should never write below
the stack pointer. Otherwise could it be related to
http://code.google.com/p/v8/issues/detail?id=1067?

http://codereview.chromium.org/6311010/diff/30001/src/arm/lithium-codegen-arm.cc
File src/arm/lithium-codegen-arm.cc (right):

http://codereview.chromium.org/6311010/diff/30001/src/arm/lithium-codegen-arm...
src/arm/lithium-codegen-arm.cc:337: // and the first word of the double in the
fixed part of the frame.
I know that you copied from above, but I don't like the constants 3 and -1 here.
Maybe we should have a class OptimizedFrameConstants in frames-arm.h as well.

It doesn't need to be in this change though.

http://codereview.chromium.org/6311010/diff/30001/src/arm/lithium-gap-resolve...
File src/arm/lithium-gap-resolver-arm.cc (right):

http://codereview.chromium.org/6311010/diff/30001/src/arm/lithium-gap-resolve...
src/arm/lithium-gap-resolver-arm.cc:235: // destroys ip, since that happens
before the value is read.
Maybe add something like "when we can't use ip use the low word of the double
scratch register as a temporary".