Add vrint{a,n,p,m,z} instructions to arm assembler.

Add vrint{a,n,p,m,z} instructions to arm assembler. These instructions are only available on ARMv8.

R=rodolph.perfetta@gmail.com, ulan@chromium.org, bmeurer@chromium.org, rodolph.perfetta@arm.com

Committed: https://code.google.com/p/v8/source/detail?r=25013

[sigurds, 2014-10-27 14:24:57]

I've added vrint instructions to arm assembler. I haven't tested them on real
hardware, but I'm going to figure out what's the best way to do this now.

I'm not sure if I got the encoding right; I intent to use the fix-length
encoding (A1 I believe), so please take a good look :)

[sigurds, 2014-10-28 13:37:05]

Patchset #7 (id:120001) has been deleted

[sigurds, 2014-10-28 13:39:48]

On 2014/10/27 14:24:57, sigurds wrote:
> I've added vrint instructions to arm assembler. I haven't tested them on real
> hardware, but I'm going to figure out what's the best way to do this now.
> 
> I'm not sure if I got the encoding right; I intent to use the fix-length
> encoding (A1 I believe), so please take a good look :)

Ulan ran the tests on hardware. After we fixed some bugs (see Patch Set 7) tests
passed.

[Rodolph Perfetta (ARM), 2014-10-28 19:41:41]

https://codereview.chromium.org/682643002/diff/140001/src/arm/disasm-arm.cc
File src/arm/disasm-arm.cc (right):

https://codereview.chromium.org/682643002/diff/140001/src/arm/disasm-arm.cc#n...
src/arm/disasm-arm.cc:1290: // vrintr - round according to rounding mode in
FPSCR
vrintr is not implemented by this patch, usually the disassembler only provide
support for what V8 generates (and test). So how about calling Unknown(instr);
here.

https://codereview.chromium.org/682643002/diff/140001/src/arm/disasm-arm.cc#n...
src/arm/disasm-arm.cc:1659: Format(instr, "vrinta.f32.f32 'Sd, 'Sm");
You don't implement single precision, so I would handle them as unknown
instructions.

Same apply for vrintn/p/m below.

https://codereview.chromium.org/682643002/diff/140001/test/cctest/test-assemb...
File test/cctest/test-assembler-arm.cc (right):

https://codereview.chromium.org/682643002/diff/140001/test/cctest/test-assemb...
test/cctest/test-assembler-arm.cc:1707: __ sub(fp, ip, Operand(4));
This is not necessary, you should be able to only store r4 and lr. This has no
impact on the test so up to you.

https://codereview.chromium.org/682643002/diff/140001/test/cctest/test-assemb...
test/cctest/test-assembler-arm.cc:1711: // Test vrint
nit: vrinta

https://codereview.chromium.org/682643002/diff/140001/test/cctest/test-disasm...
File test/cctest/test-disasm-arm.cc (right):

https://codereview.chromium.org/682643002/diff/140001/test/cctest/test-disasm...
test/cctest/test-disasm-arm.cc:501: COMPARE(vsqrt(d0, d0), "eeb10bc0      
vsqrt.f64 d0, d0");
nit: copy/paste error.

[sigurds, 2014-10-29 11:47:15]

Thanks for the first round of review, I addressed all your comments.

https://codereview.chromium.org/682643002/diff/140001/src/arm/disasm-arm.cc
File src/arm/disasm-arm.cc (right):

https://codereview.chromium.org/682643002/diff/140001/src/arm/disasm-arm.cc#n...
src/arm/disasm-arm.cc:1290: // vrintr - round according to rounding mode in
FPSCR
On 2014/10/28 19:41:41, Rodolph Perfetta (ARM) wrote:
> vrintr is not implemented by this patch, usually the disassembler only provide
> support for what V8 generates (and test). So how about calling Unknown(instr);
> here.

Done.

https://codereview.chromium.org/682643002/diff/140001/src/arm/disasm-arm.cc#n...
src/arm/disasm-arm.cc:1659: Format(instr, "vrinta.f32.f32 'Sd, 'Sm");
On 2014/10/28 19:41:41, Rodolph Perfetta (ARM) wrote:
> You don't implement single precision, so I would handle them as unknown
> instructions.
> 
> Same apply for vrintn/p/m below.

Done.

https://codereview.chromium.org/682643002/diff/140001/test/cctest/test-assemb...
File test/cctest/test-assembler-arm.cc (right):

https://codereview.chromium.org/682643002/diff/140001/test/cctest/test-assemb...
test/cctest/test-assembler-arm.cc:1707: __ sub(fp, ip, Operand(4));
On 2014/10/28 19:41:41, Rodolph Perfetta (ARM) wrote:
> This is not necessary, you should be able to only store r4 and lr. This has no
> impact on the test so up to you.

Done.

https://codereview.chromium.org/682643002/diff/140001/test/cctest/test-assemb...
test/cctest/test-assembler-arm.cc:1711: // Test vrint
On 2014/10/28 19:41:41, Rodolph Perfetta (ARM) wrote:
> nit: vrinta

Done.

https://codereview.chromium.org/682643002/diff/140001/test/cctest/test-disasm...
File test/cctest/test-disasm-arm.cc (right):

https://codereview.chromium.org/682643002/diff/140001/test/cctest/test-disasm...
test/cctest/test-disasm-arm.cc:501: COMPARE(vsqrt(d0, d0), "eeb10bc0      
vsqrt.f64 d0, d0");
On 2014/10/28 19:41:41, Rodolph Perfetta (ARM) wrote:
> nit: copy/paste error.

Done.

[Rodolph Perfetta, 2014-10-29 12:37:52]

rodolph.perfetta@gmail.com changed reviewers:
+ rodolph.perfetta@gmail.com

[Rodolph Perfetta, 2014-10-29 12:37:53]

LGTM with comments.

https://codereview.chromium.org/682643002/diff/180001/src/arm/assembler-arm.cc
File src/arm/assembler-arm.cc (right):

https://codereview.chromium.org/682643002/diff/180001/src/arm/assembler-arm.c...
src/arm/assembler-arm.cc:3101: int vd, d;
DCHECK(CpuFeature::IsSupported(ARMv8)); I miss that the first time sorry.

Same for all vrint below

https://codereview.chromium.org/682643002/diff/180001/src/arm/disasm-arm.cc
File src/arm/disasm-arm.cc (right):

https://codereview.chromium.org/682643002/diff/180001/src/arm/disasm-arm.cc#n...
src/arm/disasm-arm.cc:1282: if (instr->Opc3Value() & 0x2) {
You could move this check in the if above to simplify the code:

  if (... && (instr->Opc3Value() == 0x3)) {

https://codereview.chromium.org/682643002/diff/180001/src/arm/disasm-arm.cc#n...
src/arm/disasm-arm.cc:1287: Format(instr, "vrintz'cond.f32.f32 'Sd, 'Sm");
Not used by V8

[sigurds, 2014-10-29 15:25:05]

Addressed all comments; moved disasm tests to own test.

https://codereview.chromium.org/682643002/diff/180001/src/arm/assembler-arm.cc
File src/arm/assembler-arm.cc (right):

https://codereview.chromium.org/682643002/diff/180001/src/arm/assembler-arm.c...
src/arm/assembler-arm.cc:3101: int vd, d;
On 2014/10/29 12:37:53, Rodolph Perfetta wrote:
> DCHECK(CpuFeature::IsSupported(ARMv8)); I miss that the first time sorry.
> 
> Same for all vrint below

Done.

https://codereview.chromium.org/682643002/diff/180001/src/arm/disasm-arm.cc
File src/arm/disasm-arm.cc (right):

https://codereview.chromium.org/682643002/diff/180001/src/arm/disasm-arm.cc#n...
src/arm/disasm-arm.cc:1282: if (instr->Opc3Value() & 0x2) {
On 2014/10/29 12:37:53, Rodolph Perfetta wrote:
> You could move this check in the if above to simplify the code:
> 
>   if (... && (instr->Opc3Value() == 0x3)) {

Done.

https://codereview.chromium.org/682643002/diff/180001/src/arm/disasm-arm.cc#n...
src/arm/disasm-arm.cc:1287: Format(instr, "vrintz'cond.f32.f32 'Sd, 'Sm");
On 2014/10/29 12:37:53, Rodolph Perfetta wrote:
> Not used by V8

Done.

[ulan, 2014-10-30 08:59:47]

lgtm

[sigurds, 2014-10-30 11:01:00]

Committed patchset #10 (id:200001) manually as 25013 (presubmit successful).

[Torne, 2014-11-04 16:28:54]

torne@chromium.org changed reviewers:
+ torne@chromium.org

[Torne, 2014-11-04 16:28:55]

https://codereview.chromium.org/682643002/diff/200001/src/arm/simulator-arm.cc
File src/arm/simulator-arm.cc (right):

https://codereview.chromium.org/682643002/diff/200001/src/arm/simulator-arm.c...
src/arm/simulator-arm.cc:3627: dd_value = std::round(dm_value);
The calls to std::round and std::trunc added here require a C++11 standard
library, as those were not available in the std:: namespace previously. This
broke building the android webview on mac where the default C++ library is still
the crappy old one from gcc 4.2 :/

I'm investigating fixing the android build system downstream to use libc++ to
avoid issues like this in the future, but is this dependency against the current
chromium policy of not depending on C++11 library features? Does V8 follow that
policy?

I'm not insisting this be changed, just pointing it out in case it was
accidental. If it's fine, then we'll get it dealt with downstream.

[Sven Panne, 2014-11-05 08:05:00]

svenpanne@chromium.org changed reviewers:
+ svenpanne@chromium.org

[Sven Panne, 2014-11-05 08:05:00]

https://codereview.chromium.org/682643002/diff/200001/src/arm/simulator-arm.cc
File src/arm/simulator-arm.cc (right):

https://codereview.chromium.org/682643002/diff/200001/src/arm/simulator-arm.c...
src/arm/simulator-arm.cc:3627: dd_value = std::round(dm_value);
On 2014/11/04 16:28:55, Torne wrote:
> [...] I'm investigating fixing the android build system downstream to use
libc++ to
> avoid issues like this in the future, but is this dependency against the
current
> chromium policy of not depending on C++11 library features? Does V8 follow
that
> policy? [...]

Yes, we follow this policy, and normally this kind of problems shows up on our
Mac builders which use an ancient XCode. But this is ARM-specific, so it slipped
through and we should remove those calls in v8.