Subzero. ARM32. Vector lowering. Add.

Initial support for vector addition on ARM32.

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

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

[Eric Holk, 2016-01-25 20:29:49]

Description was changed from

==========
Updating test comment


Make sure the rest of the vector addition instructions work on ARM


Added support for <4 x i32> addition


Some vector addition works for ARM.


Some more test cases to assess the current state of vector support.


Add a failing test cases for vector addition.

BUG=
==========

to

==========
Initial support for vector addition on ARM32.

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

[Eric Holk, 2016-01-25 20:29:49]

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

[Eric Holk, 2016-01-25 20:31:34]

This change adds support for lowering vector addition on ARM for the supported
vector types.

[Eric Holk, 2016-01-25 20:32:35]

eholk@chromium.org changed reviewers:
+ sehr@chromium.org

[Karl, 2016-01-25 20:46:36]

https://codereview.chromium.org/1635713002/diff/1/src/IceInstARM32.cpp
File src/IceInstARM32.cpp (left):

https://codereview.chromium.org/1635713002/diff/1/src/IceInstARM32.cpp#oldcod...
src/IceInstARM32.cpp:605: // TODO(kschimpf) Figure if more cases are needed.
Please do not remove this TODO, or at least leave some TODO in (i.e. saying that
vector adds not implemented). This will make it easier to find, and know what
needs to be done for the integrated ARM assembler.

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

https://codereview.chromium.org/1635713002/diff/1/src/IceInstARM32.cpp#newcod...
src/IceInstARM32.cpp:609: assert(!Asm->needsTextFixup());
Please leave these asserts in. It is checking whether the integrated assembler
did the right thing.

Textual fixup's allows us to test the integrated ARM assembler as a "hybrid"
assembler. That is, if it hasn't been implemented (completely) in the integrated
assembler, the textual assembler (i.e. method emit()) is used.

This assert verifies that the integrated assembler is complete wrt the call to
method vaddd().

Note: the MINIMAL=1 build will automatically remove asserts.

[Eric Holk, 2016-01-25 20:56:19]

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

https://codereview.chromium.org/1635713002/diff/1/src/IceInstARM32.cpp#newcod...
src/IceInstARM32.cpp:609: assert(!Asm->needsTextFixup());
On 2016/01/25 20:46:36, Karl wrote:
> Please leave these asserts in. It is checking whether the integrated assembler
> did the right thing.
> 
> Textual fixup's allows us to test the integrated ARM assembler as a "hybrid"
> assembler. That is, if it hasn't been implemented (completely) in the
integrated
> assembler, the textual assembler (i.e. method emit()) is used.
> 
> This assert verifies that the integrated assembler is complete wrt the call to
> method vaddd().
> 
> Note: the MINIMAL=1 build will automatically remove asserts.

Thanks. I added the assert and the TODO back in.

[John, 2016-01-25 20:58:58]

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

https://codereview.chromium.org/1635713002/diff/1/src/IceInstARM32.cpp#newcod...
src/IceInstARM32.cpp:609: assert(!Asm->needsTextFixup());
On 2016/01/25 20:46:36, Karl wrote:
> Please leave these asserts in. It is checking whether the integrated assembler
> did the right thing.
> 
> Textual fixup's allows us to test the integrated ARM assembler as a "hybrid"
> assembler. That is, if it hasn't been implemented (completely) in the
integrated
> assembler, the textual assembler (i.e. method emit()) is used.
> 
> This assert verifies that the integrated assembler is complete wrt the call to
> method vaddd().
> 
> Note: the MINIMAL=1 build will automatically remove asserts.

Spoke to Karl. he misread the code. OK to leave as is.

https://codereview.chromium.org/1635713002/diff/1/src/IceTargetLoweringARM32.cpp
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1635713002/diff/1/src/IceTargetLoweringARM32....
src/IceTargetLoweringARM32.cpp:2805: // this explicitly whitelists vector
instructions we have some reason to
I would phrase this like

Explicitly whitelists vector instructions we have implemented/enabled.

https://codereview.chromium.org/1635713002/diff/1/tests_lit/assembler/arm32/v...
File tests_lit/assembler/arm32/vldr-float4.ll (right):

https://codereview.chromium.org/1635713002/diff/1/tests_lit/assembler/arm32/v...
tests_lit/assembler/arm32/vldr-float4.ll:42: 
does the lowering work for other vector types? if so, please add the other tests
here.

[Eric Holk, 2016-01-25 21:20:34]

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

https://codereview.chromium.org/1635713002/diff/1/src/IceInstARM32.cpp#newcod...
src/IceInstARM32.cpp:609: assert(!Asm->needsTextFixup());
On 2016/01/25 20:58:57, John wrote:
> On 2016/01/25 20:46:36, Karl wrote:
> > Please leave these asserts in. It is checking whether the integrated
assembler
> > did the right thing.
> > 
> > Textual fixup's allows us to test the integrated ARM assembler as a "hybrid"
> > assembler. That is, if it hasn't been implemented (completely) in the
> integrated
> > assembler, the textual assembler (i.e. method emit()) is used.
> > 
> > This assert verifies that the integrated assembler is complete wrt the call
to
> > method vaddd().
> > 
> > Note: the MINIMAL=1 build will automatically remove asserts.
> 
> Spoke to Karl. he misread the code. OK to leave as is.

Karl's complaint still seems valid. The assert was there to help him tell which
instructions he still needs to add to the assembler, removing the assert makes
it harder to do that. Correct?

https://codereview.chromium.org/1635713002/diff/1/tests_lit/assembler/arm32/v...
File tests_lit/assembler/arm32/vldr-float4.ll (right):

https://codereview.chromium.org/1635713002/diff/1/tests_lit/assembler/arm32/v...
tests_lit/assembler/arm32/vldr-float4.ll:42: 
On 2016/01/25 20:58:57, John wrote:
> does the lowering work for other vector types? if so, please add the other
tests
> here.

It looks like it works for <4 x i32>, but not for <8 x i16> or <16 x i8>.

[Karl, 2016-01-25 21:28:23]

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

https://codereview.chromium.org/1635713002/diff/1/src/IceInstARM32.cpp#newcod...
src/IceInstARM32.cpp:609: assert(!Asm->needsTextFixup());
On 2016/01/25 21:20:33, Eric Holk wrote:
> On 2016/01/25 20:58:57, John wrote:
> > On 2016/01/25 20:46:36, Karl wrote:
> > > Please leave these asserts in. It is checking whether the integrated
> assembler
> > > did the right thing.
> > > 
> > > Textual fixup's allows us to test the integrated ARM assembler as a
"hybrid"
> > > assembler. That is, if it hasn't been implemented (completely) in the
> > integrated
> > > assembler, the textual assembler (i.e. method emit()) is used.
> > > 
> > > This assert verifies that the integrated assembler is complete wrt the
call
> to
> > > method vaddd().
> > > 
> > > Note: the MINIMAL=1 build will automatically remove asserts.
> > 
> > Spoke to Karl. he misread the code. OK to leave as is.
> 
> Karl's complaint still seems valid. The assert was there to help him tell
which
> instructions he still needs to add to the assembler, removing the assert makes
> it harder to do that. Correct?

If you add back the TODO on line 605, it is sufficient to note.

[Jim Stichnoth, 2016-01-25 21:35:32]

https://codereview.chromium.org/1635713002/diff/20001/src/IceTargetLoweringAR...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1635713002/diff/20001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:2809: break;
Is it generally true that vector lowering will be basically identical to scalar
lowering, except for minor differences like _vadd instead of _add?   And it
applies as well to the more complicated stuff like mla?  If so, that's really
cool and refreshingly unlike x86.

https://codereview.chromium.org/1635713002/diff/20001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:2813: // DestTy is a non-i64 scalar.
This comment is no longer true.

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

https://codereview.chromium.org/1635713002/diff/20001/tests_lit/assembler/arm...
tests_lit/assembler/arm32/add-vec.ll:3: ; NOTE: Restricts S and D registers to
ones that will better test S/D
This test focuses on the Q registers, not S/D, right?  If so, change this
comment accordingly.  Also, I would simplify the -reg-use line to just name the
q registers.  Check with Karl on whether q10 and q11 are the best choices for
exercising the encodings.

https://codereview.chromium.org/1635713002/diff/20001/tests_lit/assembler/arm...
File tests_lit/assembler/arm32/vldr-float4.ll (right):

https://codereview.chromium.org/1635713002/diff/20001/tests_lit/assembler/arm...
tests_lit/assembler/arm32/vldr-float4.ll:3: ; NOTE: Restricts S and D registers
to ones that will better test S/D
Same comment as in the other .ll file.

https://codereview.chromium.org/1635713002/diff/20001/tests_lit/assembler/arm...
tests_lit/assembler/arm32/vldr-float4.ll:42: 
Remove trailing blank line, to placate git.

[Eric Holk, 2016-01-25 23:56:59]

https://codereview.chromium.org/1635713002/diff/20001/src/IceTargetLoweringAR...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1635713002/diff/20001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:2809: break;
On 2016/01/25 21:35:31, stichnot wrote:
> Is it generally true that vector lowering will be basically identical to
scalar
> lowering, except for minor differences like _vadd instead of _add?   And it
> applies as well to the more complicated stuff like mla?  If so, that's really
> cool and refreshingly unlike x86.

So far this seems true. I was pleasantly surprised. It looks like there might be
subtleties for things like element-wise multiply vs multiply by scalar.

https://codereview.chromium.org/1635713002/diff/20001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:2813: // DestTy is a non-i64 scalar.
On 2016/01/25 21:35:31, stichnot wrote:
> This comment is no longer true.

I'll remove the comment.

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

https://codereview.chromium.org/1635713002/diff/20001/tests_lit/assembler/arm...
tests_lit/assembler/arm32/add-vec.ll:3: ; NOTE: Restricts S and D registers to
ones that will better test S/D
On 2016/01/25 21:35:32, stichnot wrote:
> This test focuses on the Q registers, not S/D, right?  If so, change this
> comment accordingly.  Also, I would simplify the -reg-use line to just name
the
> q registers.  Check with Karl on whether q10 and q11 are the best choices for
> exercising the encodings.

I checked with Karl and he said different vector encodings aren't as important
with Q registers because there are so few of them. Still, he said using q10 and
q11 is useful to make sure register allocation is behaving correctly.

I've updated the comments and the -reg-use line to focus on Q registers.

[John, 2016-01-26 17:54:41]

lgtm

https://codereview.chromium.org/1635713002/diff/40001/tests_lit/assembler/arm...
File tests_lit/assembler/arm32/add-vec.ll (right):

https://codereview.chromium.org/1635713002/diff/40001/tests_lit/assembler/arm...
tests_lit/assembler/arm32/add-vec.ll:3: ; NOTE: Restricts Q registers to ones
that will better test Q register
I think this comment can be removed.

[Eric Holk, 2016-01-26 18:10:43]

Description was changed from

==========
Initial support for vector addition on ARM32.

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

to

==========
Initial support for vector addition on ARM32.

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

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

[Eric Holk, 2016-01-26 18:10:44]

Committed patchset #3 (id:40001) manually as
40c69b4310a961a119510bfc0c91af5b74985374 (presubmit successful).