Add VMLA (floating point) to the integrated ARM assembler.

Add VMLA (floating point) to the integrated ARM assembler.

Adds the scalar floating point versions of instruction VMLA to the
integrated ARM assembler.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4334
R=jpp@chromium.org, stichnot@chromium.org

Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=bd8e28e3b02fc830c37a94acfdd9e5585a7a2ab2

[Karl, 2016-01-26 18:56:46]

Description was changed from

==========
Add VMLA (floating point) to the integrated ARM assembler.

Adds the scalar floating point versions of instruction VMLA to the
integrated ARM assembler.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4334
==========

to

==========
Add VMLA (floating point) to the integrated ARM assembler.

Adds the scalar floating point versions of instruction VMLA to the
integrated ARM assembler.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4334
==========

[Karl, 2016-01-26 18:56:46]

kschimpf@google.com changed reviewers:
+ eholk@chromium.org, jpp@chromium.org, sehr@chromium.org, stichnot@chromium.org

[Eric Holk, 2016-01-26 19:11:13]

https://codereview.chromium.org/1634913005/diff/1/src/DartARM32/assembler_arm.h
File src/DartARM32/assembler_arm.h (right):

https://codereview.chromium.org/1634913005/diff/1/src/DartARM32/assembler_arm...
src/DartARM32/assembler_arm.h:687: #if 0
Out of curiosity, why use `#if 0` here instead of just deleting these lines? Is
it just to leave a record of where these functions have moved to?

[John, 2016-01-26 19:13:15]

lgtm

https://codereview.chromium.org/1634913005/diff/1/src/IceAssemblerARM32.cpp
File src/IceAssemblerARM32.cpp (right):

https://codereview.chromium.org/1634913005/diff/1/src/IceAssemblerARM32.cpp#n...
src/IceAssemblerARM32.cpp:1049: const Operand *OpDd, const Operand *OpDn,
incredibly ridiculous nit, optional: because C(++), you should probably do

const Operand *const

(if, for consistency, you want to keep as is, that's fine, too)

https://codereview.chromium.org/1634913005/diff/1/src/IceInstARM32.cpp
File src/IceInstARM32.cpp (right):

https://codereview.chromium.org/1634913005/diff/1/src/IceInstARM32.cpp#newcod...
src/IceInstARM32.cpp:655: Asm->vmlas(getDest(), getSrc(1), getSrc(2),
CondARM32::AL);
perhaps

assert(!Asm->needsTextFixup());

here (and below), and just emitUsingTextFixup() in de default:?

[Karl, 2016-01-26 19:15:26]

https://codereview.chromium.org/1634913005/diff/1/src/DartARM32/assembler_arm.h
File src/DartARM32/assembler_arm.h (right):

https://codereview.chromium.org/1634913005/diff/1/src/DartARM32/assembler_arm...
src/DartARM32/assembler_arm.h:687: #if 0
On 2016/01/26 19:11:13, Eric Holk wrote:
> Out of curiosity, why use `#if 0` here instead of just deleting these lines?
Is
> it just to leave a record of where these functions have moved to?

We are using the Dart source files to track what has been implemented in
subzero, and where they are implemented in Subzero. They are NOT compiled.
Hence, by "#if 0" the code, it is acting as documentation of what code from Dart
has been ported to Subzero.

[Jim Stichnoth, 2016-01-26 19:25:48]

lgtm

https://codereview.chromium.org/1634913005/diff/1/src/IceAssemblerARM32.cpp
File src/IceAssemblerARM32.cpp (right):

https://codereview.chromium.org/1634913005/diff/1/src/IceAssemblerARM32.cpp#n...
src/IceAssemblerARM32.cpp:1049: const Operand *OpDd, const Operand *OpDn,
On 2016/01/26 19:13:15, John wrote:
> incredibly ridiculous nit, optional: because C(++), you should probably do
> 
> const Operand *const
> 
> (if, for consistency, you want to keep as is, that's fine, too)

For historical bad reasons (my fault), pretty much everything of type Operand*
or subclass is declared without the "*const", even though these objects are
generally immutable once constructed.  I would probably just keep things as they
are for consistency, and possibly do one giant blast in a separate CL if
desired.

Maybe even better to operate on these objects as a const ref instead of a
pointer, since the vast majority are guaranteed not to be nullptr...

https://codereview.chromium.org/1634913005/diff/1/tests_lit/assembler/arm32/v...
File tests_lit/assembler/arm32/vmla.ll (right):

https://codereview.chromium.org/1634913005/diff/1/tests_lit/assembler/arm32/v...
tests_lit/assembler/arm32/vmla.ll:1: ; Tests that we can take advantage of the
vmla instruction for floating point
Incredibly nitty nit: The other .ll files start with "Show that ..." rather than
"Tests that ..."

https://codereview.chromium.org/1634913005/diff/1/tests_lit/assembler/arm32/v...
tests_lit/assembler/arm32/vmla.ll:57: 
remove trailing whitespace

[Karl, 2016-01-26 20:25:45]

Description was changed from

==========
Add VMLA (floating point) to the integrated ARM assembler.

Adds the scalar floating point versions of instruction VMLA to the
integrated ARM assembler.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4334
==========

to

==========
Add VMLA (floating point) to the integrated ARM assembler.

Adds the scalar floating point versions of instruction VMLA to the
integrated ARM assembler.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4334
R=jpp@chromium.org, stichnot@chromium.org

Committed:
https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a...
==========

[Karl, 2016-01-26 20:25:47]

Committed patchset #2 (id:20001) manually as
bd8e28e3b02fc830c37a94acfdd9e5585a7a2ab2 (presubmit successful).

[Karl, 2016-01-26 20:38:39]

https://codereview.chromium.org/1634913005/diff/1/src/IceAssemblerARM32.cpp
File src/IceAssemblerARM32.cpp (right):

https://codereview.chromium.org/1634913005/diff/1/src/IceAssemblerARM32.cpp#n...
src/IceAssemblerARM32.cpp:1049: const Operand *OpDd, const Operand *OpDn,
On 2016/01/26 19:25:48, stichnot wrote:
> On 2016/01/26 19:13:15, John wrote:
> > incredibly ridiculous nit, optional: because C(++), you should probably do
> > 
> > const Operand *const
> > 
> > (if, for consistency, you want to keep as is, that's fine, too)
> 
> For historical bad reasons (my fault), pretty much everything of type Operand*
> or subclass is declared without the "*const", even though these objects are
> generally immutable once constructed.  I would probably just keep things as
they
> are for consistency, and possibly do one giant blast in a separate CL if
> desired.
> 
> Maybe even better to operate on these objects as a const ref instead of a
> pointer, since the vast majority are guaranteed not to be nullptr...

For now, I won't change the signature.

https://codereview.chromium.org/1634913005/diff/1/src/IceInstARM32.cpp
File src/IceInstARM32.cpp (right):

https://codereview.chromium.org/1634913005/diff/1/src/IceInstARM32.cpp#newcod...
src/IceInstARM32.cpp:655: Asm->vmlas(getDest(), getSrc(1), getSrc(2),
CondARM32::AL);
On 2016/01/26 19:13:15, John wrote:
> perhaps
> 
> assert(!Asm->needsTextFixup());
> 
> here (and below), and just emitUsingTextFixup() in de default:?

Done.

https://codereview.chromium.org/1634913005/diff/1/tests_lit/assembler/arm32/v...
File tests_lit/assembler/arm32/vmla.ll (right):

https://codereview.chromium.org/1634913005/diff/1/tests_lit/assembler/arm32/v...
tests_lit/assembler/arm32/vmla.ll:1: ; Tests that we can take advantage of the
vmla instruction for floating point
On 2016/01/26 19:25:48, stichnot wrote:
> Incredibly nitty nit: The other .ll files start with "Show that ..." rather
than
> "Tests that ..."

Done.

https://codereview.chromium.org/1634913005/diff/1/tests_lit/assembler/arm32/v...
tests_lit/assembler/arm32/vmla.ll:57: 
On 2016/01/26 19:25:48, stichnot wrote:
> remove trailing whitespace
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 

Done.