Subzero. ARM32 Fcmp lowering.

Subzero. ARM32 Fcmp lowering.


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=2f5534f1c523eb022880360864f60ce42fb30ee0

[John, 2015-09-18 17:09:01]

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

[Jim Stichnoth, 2015-09-18 19:28:55]

Otherwise LGTM.

BTW, I'm very happy to report that the test_fcmp cross test works with O2 after
removing the vector tests.

It would be good to enable these and other cross tests as soon as practical to
keep from rotting.

https://codereview.chromium.org/1356763004/diff/80001/src/IceInstARM32.cpp
File src/IceInstARM32.cpp (right):

https://codereview.chromium.org/1356763004/diff/80001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:564: assert(isScalarIntegerType(DestTy) +
isScalarIntegerType(SrcTy) <= 1 &&
Adding bool types?  Ick.  Can you just !(A&&B) ?

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

https://codereview.chromium.org/1356763004/diff/80001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:90: // or if entries are added or deleted. The
following dummy namespaces use
s/dummy/anonymous/ ?
(or some suitable rewrite)

And BTW, it would be cool to apply the same "dummy" cleanup to the other target
lowering files.

https://codereview.chromium.org/1356763004/diff/80001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:2276: Variable *Src0 =
legalizeToReg(Inst->getSrc(0));
There's a (loosely-held, sadly) convention to name these variables something
like Src0R/Src1R to indicate that they are legalized to physical registers.

https://codereview.chromium.org/1356763004/diff/80001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:2769: static constexpr CondARM32::Cond Cond =
CondARM32::NE;
Hmm... can this just be constexpr, or does it have to be static as well?

https://codereview.chromium.org/1356763004/diff/80001/src/IceTargetLoweringAR...
File src/IceTargetLoweringARM32.def (right):

https://codereview.chromium.org/1356763004/diff/80001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.def:26: #define FCMPARM32_TABLE                      
                      \
Can you align all the continuation backslashes at column 80, like clang-format
does?

https://codereview.chromium.org/1356763004/diff/80001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/fp.cmp.ll (right):

https://codereview.chromium.org/1356763004/diff/80001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/fp.cmp.ll:54: ; ARM32: mov [[R:r[0-9]+]], #0
Here and below, it might be clearer style not to reuse the "R" tag when creating
a new regexp tag.

https://codereview.chromium.org/1356763004/diff/80001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/fp.cmp.ll:735: ; ARM32: vmovne.f32 [[SREG:s[0-9]+]]
Here and below, don't define the SREG/DREG tag since it's not used again?

[John, 2015-09-18 22:55:53]

landing.

https://codereview.chromium.org/1356763004/diff/80001/src/IceInstARM32.cpp
File src/IceInstARM32.cpp (right):

https://codereview.chromium.org/1356763004/diff/80001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:564: assert(isScalarIntegerType(DestTy) +
isScalarIntegerType(SrcTy) <= 1 &&
On 2015/09/18 19:28:55, stichnot wrote:
> Adding bool types?  Ick.  Can you just !(A&&B) ?

I prefer adding the bools, but your solution works. Done.

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

https://codereview.chromium.org/1356763004/diff/80001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:90: // or if entries are added or deleted. The
following dummy namespaces use
On 2015/09/18 19:28:55, stichnot wrote:
> s/dummy/anonymous/ ?
> (or some suitable rewrite)
> 
> And BTW, it would be cool to apply the same "dummy" cleanup to the other
target
> lowering files.

s/// Done.

The cleanup... let's call it 'Future work.' :)

https://codereview.chromium.org/1356763004/diff/80001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:2276: Variable *Src0 =
legalizeToReg(Inst->getSrc(0));
On 2015/09/18 19:28:55, stichnot wrote:
> There's a (loosely-held, sadly) convention to name these variables something
> like Src0R/Src1R to indicate that they are legalized to physical registers.

Done.

https://codereview.chromium.org/1356763004/diff/80001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:2769: static constexpr CondARM32::Cond Cond =
CondARM32::NE;
On 2015/09/18 19:28:55, stichnot wrote:
> Hmm... can this just be constexpr, or does it have to be static as well?

static doesn't hurt. I like declaring constexpr static because they really
**are** static to begin with. :)

https://codereview.chromium.org/1356763004/diff/80001/src/IceTargetLoweringAR...
File src/IceTargetLoweringARM32.def (right):

https://codereview.chromium.org/1356763004/diff/80001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.def:26: #define FCMPARM32_TABLE                      
                      \
On 2015/09/18 19:28:55, stichnot wrote:
> Can you align all the continuation backslashes at column 80, like clang-format
> does?

Done.

https://codereview.chromium.org/1356763004/diff/80001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/fp.cmp.ll (right):

https://codereview.chromium.org/1356763004/diff/80001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/fp.cmp.ll:54: ; ARM32: mov [[R:r[0-9]+]], #0
On 2015/09/18 19:28:55, stichnot wrote:
> Here and below, it might be clearer style not to reuse the "R" tag when
creating
> a new regexp tag.

Done.

https://codereview.chromium.org/1356763004/diff/80001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/fp.cmp.ll:735: ; ARM32: vmovne.f32 [[SREG:s[0-9]+]]
On 2015/09/18 19:28:55, stichnot wrote:
> Here and below, don't define the SREG/DREG tag since it's not used again?

Done.

[John, 2015-09-18 22:59:51]

Committed patchset #6 (id:100001) manually as
2f5534f1c523eb022880360864f60ce42fb30ee0 (presubmit successful).