Fixed bug in large switch tables on arm.

Arm codegen could emit const pool in the middle of jump table.

[Lasse Reichstein, 2008-12-09 15:29:47]

A smallish code review, plasee.

http://codereview.chromium.org/13290/diff/1/6
File src/utils.h (right):

http://codereview.chromium.org/13290/diff/1/6#newcode544
Line 544: ptr[0] = static_cast<byte>(value >> 8);
Found to be necessary when compiling with --simulator=arm on Windows.
Store32 below is apparently not used.

[Lasse Reichstein, 2008-12-09 15:46:04]

http://codereview.chromium.org/13290/diff/1/6
File src/utils.h (right):

http://codereview.chromium.org/13290/diff/1/6#newcode544
Line 544: ptr[0] = static_cast<byte>(value >> 8);
Ignore this, was also fixed by sgjesse.

[iposva, 2008-12-09 16:13:17]

Please fix the comments, otherwise LGTM.

Nice catch,
-Ivan

http://codereview.chromium.org/13290/diff/1/4
File src/macro-assembler-arm.cc (right):

http://codereview.chromium.org/13290/diff/1/4#newcode179
Line 179: void MacroAssembler::SmiJumpTable(Register index, Vector<Label*>
targets) {
The parameter index is not used within this function. Also it seems that r0 is
used directly.

http://codereview.chromium.org/13290/diff/1/4#newcode182
Line 182: add(pc, pc, Operand(r0, LSL, 2 - kSmiTagSize));
Can you document the constant 2 here? Or even better use something like
kInstrSizeLog2 which you should define in constants-arm.h next to kInstrSize.

http://codereview.chromium.org/13290/diff/1/4#newcode184
Line 184: stop("Unreachable: jump table alignment");
Please use a nop() instruction here. The stop macro is not guaranteed to be
encoded in one instruction.

[iposva, 2008-12-09 16:16:11]

One more thing. Can you please also add a testcase that triggered the bad
behaviour so that we do not regress in the future.

Thanks,
-Ivan


On 2008/12/09 16:13:17, iposva wrote:
> Please fix the comments, otherwise LGTM.
> 
> Nice catch,
> -Ivan
> 
> http://codereview.chromium.org/13290/diff/1/4
> File src/macro-assembler-arm.cc (right):
> 
> http://codereview.chromium.org/13290/diff/1/4#newcode179
> Line 179: void MacroAssembler::SmiJumpTable(Register index, Vector<Label*>
> targets) {
> The parameter index is not used within this function. Also it seems that r0 is
> used directly.
> 
> http://codereview.chromium.org/13290/diff/1/4#newcode182
> Line 182: add(pc, pc, Operand(r0, LSL, 2 - kSmiTagSize));
> Can you document the constant 2 here? Or even better use something like
> kInstrSizeLog2 which you should define in constants-arm.h next to kInstrSize.
> 
> http://codereview.chromium.org/13290/diff/1/4#newcode184
> Line 184: stop("Unreachable: jump table alignment");
> Please use a nop() instruction here. The stop macro is not guaranteed to be
> encoded in one instruction.