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

Issue 1266263003: Add the ARM32 FP register table entries, simple arith, and args. (Closed)

Created:
5 years, 4 months ago by jvoung (off chromium)
Modified:
5 years, 4 months ago
Reviewers:
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

Add the ARM32 FP register table entries, simple arith, and args. Lower some instructions, without much guarantee of correctness. *Running* generated code will be risky because the register allocator isn't aware of register aliasing. Fill in v{add,div,mul,sub}.f{32,64}, vmov, vldr and vsqrt.f{32,64}. I tried to make the nacl-other-intrinsics test not explode, so added vsqrt too. That was pretty easy for sqrt, but then fabs tests also exploded. Those are not truly fixed but are currently "fixed" by adding a FakeDef to satisfy liveness. Propagate float/double arguments to the right register in lowerArguments, lowerCall, and propagate to s0/d0/q0 for lowerReturn. May need to double check the calling convention. Currently can't test call-ret because vpush/vpop for prologues and epilogues isn't done. Legalize FP immediates to make the nacl-other-intrinsics sqrt test happy. Use the correct type of load (vldr (.32 and .64 are optional) instead of ldr{b,h,,d}). Whether or not the float/vector instructions can be predicated is a bit interesting. The float/double ones can, but the SIMD versions cannot. E.g. vadd<cond>.f32 s0, s0, s1 is okay vadd<cond>.f32 q0, q0, q1 is not okay. For now, just omit conditions from instructions that may end up being reused for SIMD. Split up the fp.pnacl.ll test into multiple ones so that parts of lowering can be tested incrementally. BUG= https://code.google.com/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=86ebec12680b5268fcb7082f770a26394e8b8080

Patch Set 1 #

Patch Set 2 : a little more stuff #

Patch Set 3 : split the tests #

Patch Set 4 : lower arguments to start piping stuff through #

Patch Set 5 : a bit more lowerRet and copyToReg, but need size for vadd, etc. #

Patch Set 6 : fix argument/call/return assignment #

Patch Set 7 : squelch liveness for now #

Patch Set 8 : fix ldr vs vldr, and fix legalizing immediates #

Patch Set 9 : don't put a suffix for vldr... it would be .32 instead of .f32 if we really wanted, but it's option… #

Patch Set 10 : tweak #

Total comments: 5

Patch Set 11 : line up #

Patch Set 12 : format more #

Unified diffs Side-by-side diffs Delta from patch set Stats (+2011 lines, -1405 lines) Patch
M src/IceInstARM32.h View 1 2 3 4 5 6 7 8 9 9 chunks +124 lines, -26 lines 0 comments Download
M src/IceInstARM32.cpp View 1 2 3 4 5 6 7 8 10 chunks +111 lines, -28 lines 0 comments Download
M src/IceInstARM32.def View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +226 lines, -76 lines 0 comments Download
M src/IceRegistersARM32.h View 1 chunk +73 lines, -25 lines 0 comments Download
M src/IceTargetLoweringARM32.h View 1 2 3 4 5 6 7 6 chunks +45 lines, -6 lines 0 comments Download
M src/IceTargetLoweringARM32.cpp View 1 2 3 4 5 6 7 8 29 chunks +167 lines, -61 lines 0 comments Download
A tests_lit/llvm2ice_tests/fp.arith.ll View 1 2 3 4 5 1 chunk +130 lines, -0 lines 0 comments Download
A tests_lit/llvm2ice_tests/fp.call_ret.ll View 1 2 3 4 5 6 7 8 1 chunk +99 lines, -0 lines 0 comments Download
A tests_lit/llvm2ice_tests/fp.cmp.ll View 1 2 1 chunk +523 lines, -0 lines 0 comments Download
A tests_lit/llvm2ice_tests/fp.convert.ll View 1 2 1 chunk +430 lines, -0 lines 0 comments Download
A tests_lit/llvm2ice_tests/fp.load_store.ll View 1 2 1 chunk +67 lines, -0 lines 0 comments Download
D tests_lit/llvm2ice_tests/fp.pnacl.ll View 1 2 1 chunk +0 lines, -1183 lines 0 comments Download
M tests_lit/llvm2ice_tests/nacl-other-intrinsics.ll View 1 2 3 4 5 6 7 8 4 chunks +16 lines, -0 lines 0 comments Download

Messages

Total messages: 6 (1 generated)
jvoung (off chromium)
5 years, 4 months ago (2015-08-07 00:37:34 UTC) #2
Jim Stichnoth
This otherwise looks gtm, but I'd like to hear your thoughts on the register allocation ...
5 years, 4 months ago (2015-08-07 15:11:36 UTC) #3
jvoung (off chromium)
https://codereview.chromium.org/1266263003/diff/170001/src/IceInstARM32.def File src/IceInstARM32.def (right): https://codereview.chromium.org/1266263003/diff/170001/src/IceInstARM32.def#newcode29 src/IceInstARM32.def:29: #define REGARM32_GPR_TABLE \ On 2015/08/07 15:11:36, stichnot wrote: > ...
5 years, 4 months ago (2015-08-08 00:32:44 UTC) #4
Jim Stichnoth
lgtm https://codereview.chromium.org/1266263003/diff/170001/src/IceInstARM32.def File src/IceInstARM32.def (right): https://codereview.chromium.org/1266263003/diff/170001/src/IceInstARM32.def#newcode71 src/IceInstARM32.def:71: X(Reg_s0 , 0 , "s0", 1, 0, 0, ...
5 years, 4 months ago (2015-08-09 00:10:05 UTC) #5
jvoung (off chromium)
5 years, 4 months ago (2015-08-09 14:58:40 UTC) #6
Message was sent while issue was closed.
Committed patchset #12 (id:210001) manually as
86ebec12680b5268fcb7082f770a26394e8b8080 (presubmit successful).

Powered by Google App Engine
This is Rietveld 408576698