Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(6)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 1685253003: ARM32 vector lowering: fabs, scalarize remaining arithmetic operations. (Closed)

Created:
4 years, 10 months ago by Eric Holk
Modified:
4 years, 10 months ago
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

Patch Set 1 #

Patch Set 2 : lit tests #

Patch Set 3 : Rebasing #

Patch Set 4 : More lit tests #

Total comments: 6

Patch Set 5 : More tests. Enabling crosstests. #

Total comments: 8

Patch Set 6 : Nits #

Unified diffs Side-by-side diffs Delta from patch set Stats (+451 lines, -264 lines) Patch
M Makefile.standalone View 1 2 3 4 1 chunk +1 line, -4 lines 0 comments Download
M pydir/crosstest.py View 1 2 3 4 5 1 chunk +3 lines, -7 lines 0 comments Download
M src/IceTargetLoweringARM32.cpp View 1 2 3 4 1 chunk +7 lines, -1 line 0 comments Download
M tests_lit/assembler/arm32/asr.ll View 1 2 3 4 5 2 chunks +68 lines, -1 line 0 comments Download
A + tests_lit/assembler/arm32/div-vec.ll View 1 1 chunk +117 lines, -0 lines 0 comments Download
M tests_lit/assembler/arm32/lsl.ll View 1 2 3 4 1 chunk +66 lines, -0 lines 0 comments Download
M tests_lit/assembler/arm32/lsr.ll View 1 2 3 4 1 chunk +66 lines, -0 lines 0 comments Download
A tests_lit/assembler/arm32/rem-vec.ll View 1 2 3 4 5 1 chunk +106 lines, -0 lines 0 comments Download
D tests_lit/assembler/arm32/udiv-vec.ll View 1 1 chunk +0 lines, -250 lines 0 comments Download
M tests_lit/assembler/arm32/vabs.ll View 1 2 3 4 5 3 chunks +17 lines, -1 line 0 comments Download

Messages

Total messages: 8 (2 generated)
Eric Holk
This patch scalarizes the remaining vector operations. Once Karl's vabs on Q-registers patch lands, we'll ...
4 years, 10 months ago (2016-02-17 00:42:30 UTC) #2
Jim Stichnoth
It would be great if we could wait for fabs and then include enabling cross ...
4 years, 10 months ago (2016-02-17 04:15:44 UTC) #3
Eric Holk
https://codereview.chromium.org/1685253003/diff/60001/tests_lit/assembler/arm32/asr.ll File tests_lit/assembler/arm32/asr.ll (right): https://codereview.chromium.org/1685253003/diff/60001/tests_lit/assembler/arm32/asr.ll#newcode65 tests_lit/assembler/arm32/asr.ll:65: define internal <4 x i32> @AshrVec(<4 x i32> %a, ...
4 years, 10 months ago (2016-02-17 19:32:16 UTC) #4
Jim Stichnoth
lgtm https://codereview.chromium.org/1685253003/diff/80001/pydir/crosstest.py File pydir/crosstest.py (right): https://codereview.chromium.org/1685253003/diff/80001/pydir/crosstest.py#newcode33 pydir/crosstest.py:33: # are more confident. +/- 4095 is the ...
4 years, 10 months ago (2016-02-17 20:59:51 UTC) #5
Eric Holk
Committed patchset #6 (id:100001) manually as 916e37ba46037f5b9590cfe8055e769099d31147 (presubmit successful).
4 years, 10 months ago (2016-02-17 21:03:32 UTC) #7
Eric Holk
4 years, 10 months ago (2016-02-17 22:31:56 UTC) #8
Message was sent while issue was closed. 
I forgot to mail these out earlier.

https://codereview.chromium.org/1685253003/diff/80001/pydir/crosstest.py
File pydir/crosstest.py (right):

https://codereview.chromium.org/1685253003/diff/80001/pydir/crosstest.py#newc...
pydir/crosstest.py:33: # are more confident. +/- 4095 is the limit, so test
On 2016/02/17 20:59:51, stichnot wrote:
> While you're at it, I would reword this comment a bit.
> 
> I think we're plenty confident by now, but we might as well retain this flag
to
> get better test coverage.  So maybe this:
> 
> # For ARM, test a large stack offset as well, to increase coverage on that
code
> path.  +/- 4096 is ...

Done.

https://codereview.chromium.org/1685253003/diff/80001/tests_lit/assembler/arm...
File tests_lit/assembler/arm32/asr.ll (right):

https://codereview.chromium.org/1685253003/diff/80001/tests_lit/assembler/arm...
tests_lit/assembler/arm32/asr.ll:1: ; Show that we know how to translate asr
On 2016/02/17 20:59:51, stichnot wrote:
> end sentence with period. (for consistency with other tests, I think)

Done.

https://codereview.chromium.org/1685253003/diff/80001/tests_lit/assembler/arm...
File tests_lit/assembler/arm32/rem-vec.ll (right):

https://codereview.chromium.org/1685253003/diff/80001/tests_lit/assembler/arm...
tests_lit/assembler/arm32/rem-vec.ll:107: 
On 2016/02/17 20:59:51, stichnot wrote:
> remove trailing blank line

Done.

https://codereview.chromium.org/1685253003/diff/80001/tests_lit/assembler/arm...
File tests_lit/assembler/arm32/vabs.ll (right):

https://codereview.chromium.org/1685253003/diff/80001/tests_lit/assembler/arm...
tests_lit/assembler/arm32/vabs.ll:1: ; Show that we translate intrinsics for
fabs on float, double and
On 2016/02/17 20:59:51, stichnot wrote:
> reflow comment to 80-col
> (unless that was meant to be a non-breaking space)

Done.

Powered by Google App Engine
This is Rietveld 408576698