Add vfp support on ARM. Patch from John Jozwiak.

Add vfp support on ARM.  Patch from John Jozwiak.

[Erik Corry, 2009-11-03 12:43:04]

This is great stuff. I am very much looking forward to getting this into V8.  I
have some formatting issues, but they are just minor things, needed to keep the
code easily readable.  Sorry about the annoyance.

The biggest issue is that the /proc reading code needs to go in
platform-linux.cc since it cannot be expected to work anywhere else and other
platforms will have different mechanisms for the same thing.

http://codereview.chromium.org/348019/diff/1/11
File src/arm/assembler-arm.cc (right):

http://codereview.chromium.org/348019/diff/1/11#newcode86
Line 86: Register  s0 = { 16-16};  // THE VFP REGISTERS   s0  to s32 (d0 to d16)
I don't think that writing 16-16 makes things clearer than writing 0.  Also,
please leave a space after the 0 and remove the double space.

http://codereview.chromium.org/348019/diff/1/11#newcode267
Line 267: B6 = 1 << 6,
Please put these in the correct order with the existing B constants.

http://codereview.chromium.org/348019/diff/1/11#newcode1339
Line 1339: void Assembler::fmdrr(const Register dst,  // support for VFP
Comments should only be put on the right if they apply to one line.  Since the
dst parameter does not support VFP more than the others the comment should go
above the function.

http://codereview.chromium.org/348019/diff/1/11#newcode1366
Line 1366: emit(cond | (0xC)*B24 | B22 | B20 | dst2.code()*B16 |
Please omit brackets around 0xC.  This applies throughout this section.

http://codereview.chromium.org/348019/diff/1/11#newcode1408
Line 1408: 
2 lines between functions.

http://codereview.chromium.org/348019/diff/1/11#newcode1498
Line 1498: void Assembler::vmrs(/*Register dst, */ Condition cond) {
This looks like it's not quite done.  There should be a dst register argument to
this function, I assume.

http://codereview.chromium.org/348019/diff/1/5
File src/arm/assembler-arm.h (right):

http://codereview.chromium.org/348019/diff/1/5#newcode104
Line 104: extern Register s0;  // Added VFP REGISTERS   s0  to s32 (d0 to d16)
The comments should document the state of the code, rather than the history of
it, so the word "Added" should be removed.  We try to follow standard English
sentence syntax in the comments so "registers" should be lower case, extra
spaces should be removed, the comment should start with a capital letter and end
with a period.  Although not all our existing code conforms to this we require
it for new code.

http://codereview.chromium.org/348019/diff/1/5#newcode697
Line 697: // adding new code for VFP
adding new code for VFP -> Support for VFP.

http://codereview.chromium.org/348019/diff/1/5#newcode698
Line 698: // all these API's support S0 to S31 and D0 to D15
all -> All
D15 -> D15.
I won't point out these comment syntax changes throughout, but they apply
everywhere.  Also double spaces should be single unless there is a reason.
API's -> APIs (grocers' apostrophe - see the rather rude summary on
http://www.angryflower.com/bobsqu.gif )

http://codereview.chromium.org/348019/diff/1/5#newcode699
Line 699: // Currently  these API do not support extended D registers, i.e, D16
to D31
API -> APIs

http://codereview.chromium.org/348019/diff/1/5#newcode700
Line 700: // Some simple modification can allow these API to support D16 to D31.
modification -> modifications
these API -> this API

http://codereview.chromium.org/348019/diff/1/5#newcode705
Line 705: const SBit  s = LeaveCC,
Superfluous double space.

http://codereview.chromium.org/348019/diff/1/5#newcode713
Line 713: const SBit s = LeaveCC, const Condition cond = al);
Please start a new line after every comma unless the whole declaration fits on
one line.

http://codereview.chromium.org/348019/diff/1/5#newcode715
Line 715: const SBit s = LeaveCC, const Condition cond = al);
indentation problem

http://codereview.chromium.org/348019/diff/1/5#newcode734
Line 734: const  SBit     s = LeaveCC,
Superfluous multiple spaces.

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

http://codereview.chromium.org/348019/diff/1/9#newcode44
Line 44: extern bool  armv7_snapdragon ;  //  see cpu-arm.cc
The right way to do this is expemplified by the class  CpuFeatures in
assembler-ia32.h.  The name of the feature should be something that mentions vfp
and doesn't mention snapdragon.

http://codereview.chromium.org/348019/diff/1/9#newcode4697
Line 4697: // __ fmrrd_r0_r1_d7();
Nonfunctional code should be removed, not just commented out.

http://codereview.chromium.org/348019/diff/1/9#newcode4754
Line 4754: 
The blank line shouldn't be removed.

http://codereview.chromium.org/348019/diff/1/9#newcode4913
Line 4913: __ mov(pc, Operand(lr), LeaveCC, eq);
My intuition says that removing the two conditional moves to pc would make
things faster.  Did you try with and without?

http://codereview.chromium.org/348019/diff/1/9#newcode5026
Line 5026: // new code added to use VFP
This comment is historical.  It can be removed when the feature has a name that
explicitly mentions VFP.

http://codereview.chromium.org/348019/diff/1/9#newcode5029
Line 5029: __ mov(r7, Operand(r7, ASR, kSmiTagSize));
These 2 instructions can be made into one.

http://codereview.chromium.org/348019/diff/1/9#newcode5036
Line 5036: __ mov(r7, Operand(r7, ASR, kSmiTagSize));
And again.

http://codereview.chromium.org/348019/diff/1/9#newcode5041
Line 5041: // This unconditional portion takes care of the original
implementation
The word 'unconditional' makes no sense here and the word original is
historical.

http://codereview.chromium.org/348019/diff/1/9#newcode5094
Line 5094: __ mov(r7, Operand(r7, ASR, kSmiTagSize));
The same comments apply to this section as to the one above.

http://codereview.chromium.org/348019/diff/1/9#newcode5131
Line 5131: __ mov(r7, Operand(r7, ASR, kSmiTagSize));
And here.

http://codereview.chromium.org/348019/diff/1/9#newcode5158
Line 5158: (Token::SUB == operation) ) ) {
The spaces after '(' and before ')' should be removed.

http://codereview.chromium.org/348019/diff/1/9#newcode5173
Line 5173: return;  //  rather than equivalent if-then-else for minimal diff.
The size of the diff takes second place to the readability of the final code. 
In this case some people prefer an else clause and some prefer an early return,
but the comment should go away.

http://codereview.chromium.org/348019/diff/1/9#newcode5235
Line 5235: __ b(gt, slow);
Can't we move the vfp code up to here.  It seems like once we know that the
number is within range we don't need the bit fiddling between here and the if
(armv7...

http://codereview.chromium.org/348019/diff/1/9#newcode5261
Line 5261: // Shift up the mantissa bits to take up the space the exponent had
taken.
I preferred the old version of the comment here.

http://codereview.chromium.org/348019/diff/1/9#newcode5298
Line 5298: // GetInt32(masm, r1, r3, r4, r5, &slow);
This looks like something left over from editing.

http://codereview.chromium.org/348019/diff/1/9#newcode5310
Line 5310: GetInt32(masm, r0, r2, r5, r4, &slow);
And here.

http://codereview.chromium.org/348019/diff/1/3
File src/arm/constants-arm.h (right):

http://codereview.chromium.org/348019/diff/1/3#newcode338
Line 338: // not supported now
Nonfunctional code should be removed, not commented out.

http://codereview.chromium.org/348019/diff/1/3#newcode344
Line 344: // static const RegisterAlias aliases_[];
And here.

http://codereview.chromium.org/348019/diff/1/8
File src/arm/cpu-arm.cc (right):

http://codereview.chromium.org/348019/diff/1/8#newcode44
Line 44: bool  armv7_snapdragon = false;
I'm not happy about this.  If there is no other way to get the information then
you should define a new function in platform.h and implement it in
platform-linux.cc.  The other platforms can get a version that returns false
always, and the whole thing should be in an ifdef that ensures that it returns
true on non-ARM (for the simulator).  In addition the formatting here is messy
and it is a sad reflection on our linting tool that it passed.  There should
generally not be multiple spaces unless they are needed for indentation.  There
should be no space after '(' and no space before ')'. There is no space before
the * in a type.

http://codereview.chromium.org/348019/diff/1/6
File src/arm/disasm-arm.cc (right):

http://codereview.chromium.org/348019/diff/1/6#newcode189
Line 189: Print(assembler::arm::VFPRegisters::Name(reg+32));
spaces around '+'

http://codereview.chromium.org/348019/diff/1/6#newcode347
Line 347: if (format[0] == 'S') PrintSRegister(((reg<<1) | instr->NField()));
Spaces around <<

http://codereview.chromium.org/348019/diff/1/6#newcode828
Line 828: // Coprocessor instructions currently not supported.
Please remove these two comment lines.

http://codereview.chromium.org/348019/diff/1/6#newcode838
Line 838: // Unknown(instr);
And these

http://codereview.chromium.org/348019/diff/1/6#newcode906
Line 906: The Following VFPv3 instructions are currently supported
I think a simple list would be easier on the eye and no less useful.

http://codereview.chromium.org/348019/diff/1/4
File src/arm/simulator-arm.cc (right):

http://codereview.chromium.org/348019/diff/1/4#newcode451
Line 451: inv_op_vfp_flag_= false;
Missing spaces before '='

http://codereview.chromium.org/348019/diff/1/4#newcode584
Line 584: void Simulator::set_s_register_from_float(int sreg, const float& flt)
{
I see no reason to use a reference here.  Small objects like floats and ints can
be passed faster by value.  This applies to the following functions too.

http://codereview.chromium.org/348019/diff/1/4#newcode586
Line 586: // now need to read the bits from the single precision floating point
value
Please remove 'now need to' from this comment and the following ones.

http://codereview.chromium.org/348019/diff/1/4#newcode899
Line 899: // i.e., if (val1 > val2)
indented 1 space too far.

http://codereview.chromium.org/348019/diff/1/4#newcode1809
Line 1809: // UNIMPLEMENTED();
Please remove comment

http://codereview.chromium.org/348019/diff/1/4#newcode1816
Line 1816: // Format(instr, "swi 'swi");
And here (not your fault).

http://codereview.chromium.org/348019/diff/1/4#newcode1894
Line 1894: The Following VFPv3 instructions are currently supported
Please just list the instructions.

http://codereview.chromium.org/348019/diff/1/7
File src/arm/simulator-arm.h (right):

http://codereview.chromium.org/348019/diff/1/7#newcode27
Line 27: 
This extra blank line is not needed.

http://codereview.chromium.org/348019/diff/1/7#newcode86
Line 86: /*
Please remove dead code.

http://codereview.chromium.org/348019/diff/1/7#newcode135
Line 135: void set_s_register_from_sinteger(int reg,  const int& value);
Extra space.

http://codereview.chromium.org/348019/diff/1/7#newcode186
Line 186: // adding the VFP architecture
Historical comment.