Subzero. ARM32. De-scalarizes icmp and fcmp for vectors.

Subzero. ARM32. De-scalarizes icmp and fcmp for vectors.

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

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

[John, 2016-04-18 16:25:32]

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

[Eric Holk, 2016-04-18 17:13:18]

lgtm with optional nits

https://codereview.chromium.org/1891243002/diff/120001/src/IceInstARM32.cpp
File src/IceInstARM32.cpp (right):

https://codereview.chromium.org/1891243002/diff/120001/src/IceInstARM32.cpp#n...
src/IceInstARM32.cpp:747: case InstARM32::FS_None: // defaults to unsigned.
It seems strange that we just asserted Sign != FS_None and then here include a
case for it in the switch statement.

https://codereview.chromium.org/1891243002/diff/120001/src/IceInstARM32.cpp#n...
src/IceInstARM32.cpp:779: case InstARM32::FS_None: // defaults to unsigned.
Same comment as above.

https://codereview.chromium.org/1891243002/diff/120001/src/IceInstARM32.cpp#n...
src/IceInstARM32.cpp:951: // TODO(jpp): remove vectors of i1.
Maybe reword this to something like "handle i1 vectors in terms of element count
instead of element type."

[Jim Stichnoth, 2016-04-18 17:34:30]

lgtm

https://codereview.chromium.org/1891243002/diff/120001/src/IceInstARM32.h
File src/IceInstARM32.h (right):

https://codereview.chromium.org/1891243002/diff/120001/src/IceInstARM32.h#new...
src/IceInstARM32.h:432: Vceq,
sort?

[John, 2016-04-18 17:48:03]

https://codereview.chromium.org/1891243002/diff/120001/src/IceInstARM32.cpp
File src/IceInstARM32.cpp (right):

https://codereview.chromium.org/1891243002/diff/120001/src/IceInstARM32.cpp#n...
src/IceInstARM32.cpp:747: case InstARM32::FS_None: // defaults to unsigned.
On 2016/04/18 17:13:17, Eric Holk wrote:
> It seems strange that we just asserted Sign != FS_None and then here include a
> case for it in the switch statement.

well, the case is needed anyways -- otherwise clang will complain about an
unhandled enum value. I added an llvm_unreachable.

https://codereview.chromium.org/1891243002/diff/120001/src/IceInstARM32.cpp#n...
src/IceInstARM32.cpp:779: case InstARM32::FS_None: // defaults to unsigned.
On 2016/04/18 17:13:17, Eric Holk wrote:
> Same comment as above.

Done.

https://codereview.chromium.org/1891243002/diff/120001/src/IceInstARM32.cpp#n...
src/IceInstARM32.cpp:951: // TODO(jpp): remove vectors of i1.
On 2016/04/18 17:13:17, Eric Holk wrote:
> Maybe reword this to something like "handle i1 vectors in terms of element
count
> instead of element type."

Done.

https://codereview.chromium.org/1891243002/diff/120001/src/IceInstARM32.h
File src/IceInstARM32.h (right):

https://codereview.chromium.org/1891243002/diff/120001/src/IceInstARM32.h#new...
src/IceInstARM32.h:432: Vceq,
On 2016/04/18 17:34:29, stichnot wrote:
> sort?

Done.

[John, 2016-04-18 22:32:30]

Description was changed from

==========
Subzero. ARM32. De-scalarizes icmp and fcmp for vectors.

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

to

==========
Subzero. ARM32. De-scalarizes icmp and fcmp for vectors.

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

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

[John, 2016-04-18 22:32:31]

Committed patchset #8 (id:140001) manually as
a4d100ab21814f39591684c59e4c4e113fa0f1f4 (presubmit successful).