Add support for XMM8..XMM15

Add support for XMM8..XMM15

Committed: https://code.google.com/p/dart/source/detail?r=15162

[Cutch, 2012-11-15 04:05:42]

There remains a bug in this CL but it's close.

[Cutch, 2012-11-15 04:07:13]

On 2012/11/15 04:05:42, Cutch wrote:
> There remains a bug in this CL but it's close.

Specifically mandel_isolate_test.dart is failing with an illegal instruction.
I'll look into it tomorrow.

[Florian Schneider, 2012-11-15 05:40:59]

dbc: Please also extend the assembler tests for the new instructions in
assembler_x64_test.cc

I just remembered that I have a CL for this that is almost complete, but got
dropped into the gaps somehow. Let me see if I can dig it up and we can merge
it.

https://codereview.chromium.org/11358262/diff/1/runtime/vm/assembler_x64.cc
File runtime/vm/assembler_x64.cc (right):

https://codereview.chromium.org/11358262/diff/1/runtime/vm/assembler_x64.cc#n...
runtime/vm/assembler_x64.cc:424: EmitXmmRegisterREX(src, REX_NONE);
I think you also need to set the REX_B bit for register-register addressing.

Maybe the helper should take two registers like:

EmitXmmRegisterREX(Register reg, Register rm, uint8_t rex)

[Cutch, 2012-11-16 18:59:43]

Patch Set 2 is ready for review. All tests pass.

[Florian Schneider, 2012-11-19 10:34:33]

Thanks! LGTM with comments.

https://codereview.chromium.org/11358262/diff/5001/runtime/vm/assembler_x64.cc
File runtime/vm/assembler_x64.cc (right):

https://codereview.chromium.org/11358262/diff/5001/runtime/vm/assembler_x64.c...
runtime/vm/assembler_x64.cc:431: void Assembler::movd(XmmRegister dst, Register
src) {
Please extends the tests for movd to also use registers >= XMM8.

https://codereview.chromium.org/11358262/diff/5001/runtime/vm/assembler_x64.c...
runtime/vm/assembler_x64.cc:453: void Assembler::addss(XmmRegister dst,
XmmRegister src) {
Please also extend the tests to cover the single-precision ops (addss, subss,
mulss, divss)

https://codereview.chromium.org/11358262/diff/5001/runtime/vm/assembler_x64.c...
runtime/vm/assembler_x64.cc:652: void Assembler::cvtsi2sd(XmmRegister dst,
Register src) {
Please also extends the tests to use a register >= XMM8 for this one.

https://codereview.chromium.org/11358262/diff/5001/runtime/vm/assembler_x64.c...
runtime/vm/assembler_x64.cc:669: EmitOperandREX(0, operand, REX_W);
I think you need the following here (please add a test to confirm):

EmitREX_RB(dst, src, REX_W);

https://codereview.chromium.org/11358262/diff/5001/runtime/vm/assembler_x64.c...
runtime/vm/assembler_x64.cc:672: EmitXmmRegisterOperand(dst, src);
And also

dst & 7

https://codereview.chromium.org/11358262/diff/5001/runtime/vm/assembler_x64_t...
File runtime/vm/assembler_x64_test.cc (right):

https://codereview.chromium.org/11358262/diff/5001/runtime/vm/assembler_x64_t...
runtime/vm/assembler_x64_test.cc:1399: __ sqrtsd(XMM8, XMM12);  // 1.843
Maybe do sqrtsd(XMM8, XMM8) instead to include the result of the previous
operations.

https://codereview.chromium.org/11358262/diff/5001/runtime/vm/assembler_x64_t...
runtime/vm/assembler_x64_test.cc:1405: __ sqrtsd(XMM0, XMM1);  // 1.843
Maybe do sqrtsd(XMM0, XMM0) instead to include the result of the previous
operations.

To check correctness of XMM8, you could do subsd(XMM0, XMM8) here and check the
expected result value against 0.0.

[Cutch, 2012-11-19 23:28:58]

https://codereview.chromium.org/11358262/diff/5001/runtime/vm/assembler_x64.cc
File runtime/vm/assembler_x64.cc (right):

https://codereview.chromium.org/11358262/diff/5001/runtime/vm/assembler_x64.c...
runtime/vm/assembler_x64.cc:431: void Assembler::movd(XmmRegister dst, Register
src) {
On 2012/11/19 10:34:33, Florian Schneider wrote:
> Please extends the tests for movd to also use registers >= XMM8.

Done.

https://codereview.chromium.org/11358262/diff/5001/runtime/vm/assembler_x64.c...
runtime/vm/assembler_x64.cc:431: void Assembler::movd(XmmRegister dst, Register
src) {
On 2012/11/19 10:34:33, Florian Schneider wrote:
> Please extends the tests for movd to also use registers >= XMM8.

Done.

https://codereview.chromium.org/11358262/diff/5001/runtime/vm/assembler_x64.c...
runtime/vm/assembler_x64.cc:453: void Assembler::addss(XmmRegister dst,
XmmRegister src) {
On 2012/11/19 10:34:33, Florian Schneider wrote:
> Please also extend the tests to cover the single-precision ops (addss, subss,
> mulss, divss)

Done.

https://codereview.chromium.org/11358262/diff/5001/runtime/vm/assembler_x64.c...
runtime/vm/assembler_x64.cc:652: void Assembler::cvtsi2sd(XmmRegister dst,
Register src) {
On 2012/11/19 10:34:33, Florian Schneider wrote:
> Please also extends the tests to use a register >= XMM8 for this one.

Done.

https://codereview.chromium.org/11358262/diff/5001/runtime/vm/assembler_x64.c...
runtime/vm/assembler_x64.cc:669: EmitOperandREX(0, operand, REX_W);
Yes. Test confirmed this was broken. Fixed now.

On 2012/11/19 10:34:33, Florian Schneider wrote:
> I think you need the following here (please add a test to confirm):
> 
> EmitREX_RB(dst, src, REX_W);

https://codereview.chromium.org/11358262/diff/5001/runtime/vm/assembler_x64.c...
runtime/vm/assembler_x64.cc:672: EmitXmmRegisterOperand(dst, src);
On 2012/11/19 10:34:33, Florian Schneider wrote:
> And also
> 
> dst & 7

Done.

https://codereview.chromium.org/11358262/diff/5001/runtime/vm/assembler_x64.c...
runtime/vm/assembler_x64.cc:672: EmitXmmRegisterOperand(dst, src);
On 2012/11/19 10:34:33, Florian Schneider wrote:
> And also
> 
> dst & 7

Done.

https://codereview.chromium.org/11358262/diff/5001/runtime/vm/assembler_x64_t...
File runtime/vm/assembler_x64_test.cc (right):

https://codereview.chromium.org/11358262/diff/5001/runtime/vm/assembler_x64_t...
runtime/vm/assembler_x64_test.cc:1399: __ sqrtsd(XMM8, XMM12);  // 1.843
On 2012/11/19 10:34:33, Florian Schneider wrote:
> Maybe do sqrtsd(XMM8, XMM8) instead to include the result of the previous
> operations.

Done.

https://codereview.chromium.org/11358262/diff/5001/runtime/vm/assembler_x64_t...
runtime/vm/assembler_x64_test.cc:1399: __ sqrtsd(XMM8, XMM12);  // 1.843
Done, I also add the XMM0 and XMM8 values together at the end.
On 2012/11/19 10:34:33, Florian Schneider wrote:
> Maybe do sqrtsd(XMM8, XMM8) instead to include the result of the previous
> operations.

https://codereview.chromium.org/11358262/diff/5001/runtime/vm/assembler_x64_t...
runtime/vm/assembler_x64_test.cc:1405: __ sqrtsd(XMM0, XMM1);  // 1.843
I add them together. I use the subtraction op elsewhere.
On 2012/11/19 10:34:33, Florian Schneider wrote:
> Maybe do sqrtsd(XMM0, XMM0) instead to include the result of the previous
> operations.
> 
> To check correctness of XMM8, you could do subsd(XMM0, XMM8) here and check
the
> expected result value against 0.0.

[Florian Schneider, 2012-11-20 12:31:12]

LGTM.