Chromium Code Reviews

Issue 1683153003: ARM32 vector ops - scalarize icmp, fcmp and cast. (Closed)

Created:
4 years, 10 months ago by Eric Holk
Modified:
4 years, 10 months ago
Reviewers:
kschimpf, sehr, Jim Stichnoth, John
CC:
native-client-reviews_googlegroups.com
Base URL:
https://chromium.googlesource.com/native_client/pnacl-subzero.git@master
Target Ref:
refs/heads/master
Visibility:
Public.

Description

ARM32 vector ops - scalarize icmp, fcmp and cast. This is part of a sequence of patches to quickly fill out vector support by scalarizing the remaining operations. Later we can work to generate better code. BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4076 R=stichnot@chromium.org Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=cc69fa29cf14a3c4340f1ea4a32a1d24772b605d

Patch Set 1 #

Patch Set 2 : Adding tests for vector icmp and fcmp #

Total comments: 8
Unified diffs Side-by-side diffs Stats (+270 lines, -111 lines)
M src/IceInstARM32.cpp View 2 chunks +6 lines, -0 lines 2 comments
M src/IceTargetLowering.h View 1 chunk +77 lines, -0 lines 4 comments
M src/IceTargetLowering.cpp View 1 chunk +6 lines, -28 lines 0 comments
M src/IceTargetLoweringARM32.cpp View 4 chunks +47 lines, -5 lines 0 comments
A tests_lit/assembler/arm32/cmp-vec.ll View 1 chunk +52 lines, -0 lines 2 comments
M tests_lit/assembler/arm32/udiv-vec.ll View 4 chunks +82 lines, -78 lines 0 comments

Messages

Total messages: 6 (2 generated)
Eric Holk
4 years, 10 months ago (2016-02-10 18:31:15 UTC) #2
Jim Stichnoth
lgtm https://codereview.chromium.org/1683153003/diff/20001/src/IceInstARM32.cpp File src/IceInstARM32.cpp (right): https://codereview.chromium.org/1683153003/diff/20001/src/IceInstARM32.cpp#newcode1757 src/IceInstARM32.cpp:1757: case IceType_v8i1: make format https://codereview.chromium.org/1683153003/diff/20001/src/IceTargetLowering.h File src/IceTargetLowering.h (right): ...
4 years, 10 months ago (2016-02-10 19:08:12 UTC) #3
Eric Holk
Committed patchset #2 (id:20001) manually as cc69fa29cf14a3c4340f1ea4a32a1d24772b605d (presubmit successful).
4 years, 10 months ago (2016-02-10 21:07:09 UTC) #5
Eric Holk
4 years, 10 months ago (2016-02-10 21:07:20 UTC) #6
Message was sent while issue was closed. 
https://codereview.chromium.org/1683153003/diff/20001/src/IceInstARM32.cpp
File src/IceInstARM32.cpp (right):

https://codereview.chromium.org/1683153003/diff/20001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:1757: case  IceType_v8i1:
On 2016/02/10 19:08:11, stichnot wrote:
> make format

Done.

https://codereview.chromium.org/1683153003/diff/20001/src/IceTargetLowering.h
File src/IceTargetLowering.h (right):

https://codereview.chromium.org/1683153003/diff/20001/src/IceTargetLowering.h...
src/IceTargetLowering.h:477: assert(isVectorType(Dest->getType()));
On 2016/02/10 19:08:11, stichnot wrote:
> I would "assert(isVectorType(DestTy));" and move it one instruction below. 
> (despite the break in the flow)
> 
> here and below

Done.

https://codereview.chromium.org/1683153003/diff/20001/src/IceTargetLowering.h...
src/IceTargetLowering.h:498: // Perform the arithmetic as a scalar operation.
On 2016/02/10 19:08:12, stichnot wrote:
> You probably want change "arithmetic" to be more general now.
> 
> Here and below.

Done.

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

https://codereview.chromium.org/1683153003/diff/20001/tests_lit/assembler/arm...
tests_lit/assembler/arm32/cmp-vec.ll:1: ; Test that we handle icmp and fcmp on
vectors
On 2016/02/10 19:08:12, stichnot wrote:
> End sentence with period.  :)

Done.

Powered by Google App Engine