Implementing a few more missing stubs on ARM:...

Implementing a few more missing stubs on ARM:
 * LCodeGen::EmitClassOfTest
 * LCodeGen::DoClassOfTestAndBranch
 * LCodeGen::DoMathFloor
 * LCodeGen::DoMathSqrt

BUG=none
TEST=none

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

LGTM, with comments.

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

http://codereview.chromium.org/6124005/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:1384: // the temp registers, but not the input.
Only input and temp2 may alias.
input is trashed if temp2 aliases it.

http://codereview.chromium.org/6124005/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:1398: 
How about just

// temp:  map of input
// temp2: object type of input

http://codereview.chromium.org/6124005/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:1412: __ ldr(temp, FieldMemOperand(temp,
Map::kConstructorOffset));
After this

// temp: constructor

http://codereview.chromium.org/6124005/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:1427: 
Change comment to

// Get the instance class name from the constructor.

http://codereview.chromium.org/6124005/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:1444: void LCodeGen::DoClassOfTest(LClassOfTest*
instr) {
Maybe implement this now you have done EmitClassOfTest.

http://codereview.chromium.org/6124005/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:1454: // Swap.
There is a Swap instruction in the macro assembler.

http://codereview.chromium.org/6124005/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:1855: DoubleRegister input_reg =
ToDoubleRegister(instr->input());
input_reg -> input?

http://codereview.chromium.org/6124005/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:1856: SwVfpRegister single_precision_scratch =
s0;
Please add double_scratch0() and single_scratch0() (and perhaps
single_scratch1()) like scratch0().

http://codereview.chromium.org/6124005/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:1861: //  - Set rounding mode to "Round towards
Minus Infinity"
No need to refer to specific bit numbers in the comments.

http://codereview.chromium.org/6124005/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:1865: __ vmrs(scratch);
Don't we need to backup and restore FPSCR here? Thats done in
CallStubCompiler::CompileMathFloorCall.

http://codereview.chromium.org/6124005/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:1888: DoubleRegister input_reg =
ToDoubleRegister(instr->input());
input_reg -> input?

[Alexandre, 2011-01-10 10:48:27]

Thanks for the review.
Rodolph will address the last comments and upload the patch.

Alexandre

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

http://codereview.chromium.org/6124005/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:1384: // the temp registers, but not the input.
Only input and temp2 may alias.
Indeed. Changed the code to have input and temp2 always different.
I have a few questions however:
- The ia32 comment states that input is not trashed. Is it required?
- More generally, where can I find information on what each LCodeGen stub
requires? I can find some info in the lithium-<arch>.* files, but how do I know
for example if the stub is allowed to trash the input?
On 2011/01/10 09:56:08, Søren Gjesse wrote:
> input is trashed if temp2 aliases it.

http://codereview.chromium.org/6124005/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:1398: 
On 2011/01/10 09:56:08, Søren Gjesse wrote:
> How about just
> 
> // temp:  map of input
> // temp2: object type of input

Done.

http://codereview.chromium.org/6124005/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:1412: __ ldr(temp, FieldMemOperand(temp,
Map::kConstructorOffset));
On 2011/01/10 09:56:08, Søren Gjesse wrote:
> After this
> 
> // temp: constructor

Done.

http://codereview.chromium.org/6124005/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:1427: 
On 2011/01/10 09:56:08, Søren Gjesse wrote:
> Change comment to
> 
> // Get the instance class name from the constructor.

Done.

http://codereview.chromium.org/6124005/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:1444: void LCodeGen::DoClassOfTest(LClassOfTest*
instr) {
Implemented.
On 2011/01/10 09:56:08, Søren Gjesse wrote:
> Maybe implement this now you have done EmitClassOfTest.

http://codereview.chromium.org/6124005/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:1454: // Swap.
Removed. Not needed anymore, as all these registers should be different.
On 2011/01/10 09:56:08, Søren Gjesse wrote:
> There is a Swap instruction in the macro assembler.

http://codereview.chromium.org/6124005/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:1888: DoubleRegister input_reg =
ToDoubleRegister(instr->input());
On 2011/01/10 09:56:08, Søren Gjesse wrote:
> input_reg -> input?

Done.

[Rodolph Perfetta, 2011-01-13 11:38:48]

this is now closed, replaced by issue 6250002
http://codereview.chromium.org/6250002/