Add move vector to the integrated ARM assembler.

Add move vector to the integrated ARM assembler.

Implements a vector move as a vector orr on the corresponding Q
registers.

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

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

[Karl, 2016-02-11 19:07:30]

Description was changed from

==========
Add move vector to the integrated ARM assembler.

Implements a vector move as a vector orr on the corresponding Q
registers.

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

to

==========
Add move vector to the integrated ARM assembler.

Implements a vector move as a vector orr on the corresponding Q
registers.

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

[Karl, 2016-02-11 19:07:31]

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

[Eric Holk, 2016-02-11 19:24:22]

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

https://codereview.chromium.org/1694533002/diff/1/src/IceInstARM32.cpp#newcod...
src/IceInstARM32.cpp:1570: Asm->vorrq(Dest, Src0, Src0);
The other moves in this function pass the result of getPredicate() to the
assembler. Do we need to handle conditional vector moves? We should probably at
least assert that it's an unconditional move in case some higher level code
tries to generate one.

https://codereview.chromium.org/1694533002/diff/1/tests_lit/assembler/arm32/v...
File tests_lit/assembler/arm32/vec-move.ll (right):

https://codereview.chromium.org/1694533002/diff/1/tests_lit/assembler/arm32/v...
tests_lit/assembler/arm32/vec-move.ll:32: %0 = sitofp <4 x i32> %a to <4 x
float>
This may not always create a move. Right now this cast works by scalarization
which leads to bad code. We'll hopefully do better in the future, although if we
do I suppose we can fix the test then.

[Karl, 2016-02-11 22:10:28]

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

https://codereview.chromium.org/1694533002/diff/1/src/IceInstARM32.cpp#newcod...
src/IceInstARM32.cpp:1570: Asm->vorrq(Dest, Src0, Src0);
On 2016/02/11 19:24:22, Eric Holk wrote:
> The other moves in this function pass the result of getPredicate() to the
> assembler. Do we need to handle conditional vector moves? We should probably
at
> least assert that it's an unconditional move in case some higher level code
> tries to generate one.

The same can be said for the emit() method. Adding TODO to fix that case as
well!

I talked to John and (at least currently) we shouldn't be generating conditions
for these vector moves.

Done

https://codereview.chromium.org/1694533002/diff/1/tests_lit/assembler/arm32/v...
File tests_lit/assembler/arm32/vec-move.ll (right):

https://codereview.chromium.org/1694533002/diff/1/tests_lit/assembler/arm32/v...
tests_lit/assembler/arm32/vec-move.ll:32: %0 = sitofp <4 x i32> %a to <4 x
float>
On 2016/02/11 19:24:22, Eric Holk wrote:
> This may not always create a move. Right now this cast works by scalarization
> which leads to bad code. We'll hopefully do better in the future, although if
we
> do I suppose we can fix the test then.

Unless you can come up with a better example, I suggest we use this for now. If
some change causes the instruction to disappear, this test will fail, and it can
be fixed then.

[Eric Holk, 2016-02-11 22:51:39]

lgtm

https://codereview.chromium.org/1694533002/diff/1/tests_lit/assembler/arm32/v...
File tests_lit/assembler/arm32/vec-move.ll (right):

https://codereview.chromium.org/1694533002/diff/1/tests_lit/assembler/arm32/v...
tests_lit/assembler/arm32/vec-move.ll:32: %0 = sitofp <4 x i32> %a to <4 x
float>
On 2016/02/11 22:10:28, Karl wrote:
> On 2016/02/11 19:24:22, Eric Holk wrote:
> > This may not always create a move. Right now this cast works by
scalarization
> > which leads to bad code. We'll hopefully do better in the future, although
if
> we
> > do I suppose we can fix the test then.
> 
> Unless you can come up with a better example, I suggest we use this for now.
If
> some change causes the instruction to disappear, this test will fail, and it
can
> be fixed then.

I agree. This seems like the best course of action.

[Karl, 2016-02-11 23:20:54]

Description was changed from

==========
Add move vector to the integrated ARM assembler.

Implements a vector move as a vector orr on the corresponding Q
registers.

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

to

==========
Add move vector to the integrated ARM assembler.

Implements a vector move as a vector orr on the corresponding Q
registers.

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

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

[Karl, 2016-02-11 23:20:55]

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

[John, 2016-02-11 23:59:54]

lgtm

Just nits, no need to address.

https://codereview.chromium.org/1694533002/diff/20001/src/IceConditionCodesAR...
File src/IceConditionCodesARM32.h (right):

https://codereview.chromium.org/1694533002/diff/20001/src/IceConditionCodesAR...
src/IceConditionCodesARM32.h:39: static bool isUnconditional(Cond C) { return
!isDefined(C) || C == AL; }
Why is this not a global function in an unnamed namespace? There's no reason for
it to be part of the class' interface.

https://codereview.chromium.org/1694533002/diff/20001/src/IceInstARM32.cpp
File src/IceInstARM32.cpp (right):

https://codereview.chromium.org/1694533002/diff/20001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:1440: if (IsVector)
This is a serious error. Maybe llvm::report_fatal_error()?

https://codereview.chromium.org/1694533002/diff/20001/tests_lit/assembler/arm...
File tests_lit/assembler/arm32/vec-move.ll (right):

https://codereview.chromium.org/1694533002/diff/20001/tests_lit/assembler/arm...
tests_lit/assembler/arm32/vec-move.ll:32: %0 = sitofp <4 x i32> %a to <4 x
float>
No action required: You can always "shuffle" the parameters to force a move:

define internal <4 x i32> @test(<4 x i32> %a, <4 x i32> ℅b) {
  ret <4 x i32> ℅b;
}

[Jim Stichnoth, 2016-02-12 01:38:55]

lgtm

https://codereview.chromium.org/1694533002/diff/20001/src/IceInstARM32.cpp
File src/IceInstARM32.cpp (right):

https://codereview.chromium.org/1694533002/diff/20001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:1439: CondARM32::Cond Cond = getPredicate();
const