Subzero. Enables (most) crosstests for ARM32.

Subzero. Enables (most) crosstests for ARM32.

This patch enables many crosstests for ARM32. Very limited vector
support is implemented (essentially, whatever it takes to compile the
.ll files contain vector operations.)

Atomics as well as vector crosstests are still disabled.

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=ba6a67c960a126d85b94a87c8ea7358bec0da048

[John, 2015-09-23 22:34:42]

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

[Jim Stichnoth, 2015-09-24 05:57:14]

https://codereview.chromium.org/1359193003/diff/180001/crosstest/test_arith_m...
File crosstest/test_arith_main.cpp (right):

https://codereview.chromium.org/1359193003/diff/180001/crosstest/test_arith_m...
crosstest/test_arith_main.cpp:157: #ifndef ARM32
I'd like to see TODOs on all these "#ifndef ARM32" lines just to be clear.

https://codereview.chromium.org/1359193003/diff/180001/crosstest/test_calling...
File crosstest/test_calling_conv.h (right):

https://codereview.chromium.org/1359193003/diff/180001/crosstest/test_calling...
crosstest/test_calling_conv.h:35: typedef void(callee_vlvlivfvdviv_Ty)(v4f32,
int64, v4f32, int, int64, v4f32,
I'm not sure why you needed to change the argument order, but now the name of
the function type doesn't match the arguments.

Old: callee_vlvlivfvdviv_Ty
New: callee_vlvilvfvdviv_Ty
               ^^

https://codereview.chromium.org/1359193003/diff/180001/pydir/crosstest.py
File pydir/crosstest.py (left):

https://codereview.chromium.org/1359193003/diff/180001/pydir/crosstest.py#old...
pydir/crosstest.py:38: '--test-stack-extra', '4084']
This seems useful for full testing, so why remove it?

https://codereview.chromium.org/1359193003/diff/180001/src/IceInstARM32.cpp
File src/IceInstARM32.cpp (right):

https://codereview.chromium.org/1359193003/diff/180001/src/IceInstARM32.cpp#n...
src/IceInstARM32.cpp:214: /*if (isScalarFloatingType(Ty))
Explain or delete comment?

https://codereview.chromium.org/1359193003/diff/180001/src/IceInstARM32.h
File src/IceInstARM32.h (right):

https://codereview.chromium.org/1359193003/diff/180001/src/IceInstARM32.h#new...
src/IceInstARM32.h:785: //!!!!!!!!!!!!!!!!!!!!!!using InstARM32Mov =
InstARM32Movlike<InstARM32::Mov>;
Explain this comment, or delete it?

https://codereview.chromium.org/1359193003/diff/180001/src/IceTargetLoweringA...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1359193003/diff/180001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:612: // fp types, cannot have an index register.)
This ARM nonsense makes one
I don't think that sort of editorial comment should be in this code base...

https://codereview.chromium.org/1359193003/diff/180001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:1456: // orr     Src0RHi, T, Src0RHi, lsl Src1RLo
I don't think this is a correct abstraction of what llc does, since it's
unlikely to smash Src0RHi and Src0RLo if Src0 is live beyond the shl
instruction.  Maybe it should look more like this?

  mov t_b.lo, b.lo
  mov t_b.hi, b.hi
  mov t_c.lo, c.lo
  rsb T, t_c.lo, #32
  lsr T1, t_b.lo, T
  orr T3, T1, t_b.hi, lsl t_c.lo
  sub T2, t_c.lo, #32
  cmp T2, #0
  lslge T3, t_b.lo, T2
  lsl T5, t_b.lo, t_c.lo

https://codereview.chromium.org/1359193003/diff/180001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:1483: Variable *T1 = makeReg(IceType_i32);
Suggestion: I think the pseudo-assembly reads easier if you move the makeReg()s
and getConstant()s to the top.  (Then it will be more clear that T4 is missing,
though not so clear that T3 is defined before T2...)

https://codereview.chromium.org/1359193003/diff/180001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:1571: Variable *T = makeReg(Dest->getType());
What do you think about bundling these 4 statements into a macro or something
like that?

https://codereview.chromium.org/1359193003/diff/180001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:2354:
UnimplementedError(Func->getContext()->getFlags());
Usually the UnimplementedError() comes after the placeholder lowering?

https://codereview.chromium.org/1359193003/diff/180001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:2990: _mov(Reg, Src);
Should there be just one _mov() ?

https://codereview.chromium.org/1359193003/diff/180001/src/IceTargetLoweringA...
File src/IceTargetLoweringARM32.h (right):

https://codereview.chromium.org/1359193003/diff/180001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.h:249: /// _ldr, for all your Variable to memory data
moves. It handles all types
I would describe this as "memory to Variable data moves", right?

Essentially same comment for _str below.

https://codereview.chromium.org/1359193003/diff/180001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.h:277: /// If Dest=nullptr is passed in, then a new
variable
Delete this part of the comment?  (contradicted below)

https://codereview.chromium.org/1359193003/diff/180001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.h:283: // automagically of Dest was nullptr. It won't
do that anymore, so we keep
s/of/if/

[John, 2015-09-24 21:28:40]

https://codereview.chromium.org/1359193003/diff/180001/crosstest/test_arith_m...
File crosstest/test_arith_main.cpp (right):

https://codereview.chromium.org/1359193003/diff/180001/crosstest/test_arith_m...
crosstest/test_arith_main.cpp:157: #ifndef ARM32
On 2015/09/24 05:57:13, stichnot wrote:
> I'd like to see TODOs on all these "#ifndef ARM32" lines just to be clear.

Done.

https://codereview.chromium.org/1359193003/diff/180001/crosstest/test_calling...
File crosstest/test_calling_conv.h (right):

https://codereview.chromium.org/1359193003/diff/180001/crosstest/test_calling...
crosstest/test_calling_conv.h:35: typedef void(callee_vlvlivfvdviv_Ty)(v4f32,
int64, v4f32, int, int64, v4f32,
On 2015/09/24 05:57:13, stichnot wrote:
> I'm not sure why you needed to change the argument order, but now the name of
> the function type doesn't match the arguments.
> 
> Old: callee_vlvlivfvdviv_Ty
> New: callee_vlvilvfvdviv_Ty
>                ^^
Done.

For reference: The two int64 parameters fit nicely in r0-r1, and r2-r3. By
swapping the arguments I can test that the ARM32 target is passing the second
int64 in the stack.

https://codereview.chromium.org/1359193003/diff/180001/pydir/crosstest.py
File pydir/crosstest.py (left):

https://codereview.chromium.org/1359193003/diff/180001/pydir/crosstest.py#old...
pydir/crosstest.py:38: '--test-stack-extra', '4084']
On 2015/09/24 05:57:13, stichnot wrote:
> This seems useful for full testing, so why remove it?

Because the lowering is broken. I added a todo to revert this.

https://codereview.chromium.org/1359193003/diff/180001/src/IceInstARM32.cpp
File src/IceInstARM32.cpp (right):

https://codereview.chromium.org/1359193003/diff/180001/src/IceInstARM32.cpp#n...
src/IceInstARM32.cpp:214: /*if (isScalarFloatingType(Ty))
On 2015/09/24 05:57:13, stichnot wrote:
> Explain or delete comment?

Done.

https://codereview.chromium.org/1359193003/diff/180001/src/IceInstARM32.h
File src/IceInstARM32.h (right):

https://codereview.chromium.org/1359193003/diff/180001/src/IceInstARM32.h#new...
src/IceInstARM32.h:785: //!!!!!!!!!!!!!!!!!!!!!!using InstARM32Mov =
InstARM32Movlike<InstARM32::Mov>;
On 2015/09/24 05:57:13, stichnot wrote:
> Explain this comment, or delete it?

Done.

https://codereview.chromium.org/1359193003/diff/180001/src/IceTargetLoweringA...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1359193003/diff/180001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:612: // fp types, cannot have an index register.)
This ARM nonsense makes one
On 2015/09/24 05:57:13, stichnot wrote:
> I don't think that sort of editorial comment should be in this code base...

That's because you are not working with ARM32.. :P

Just joking. You're right. Done.

https://codereview.chromium.org/1359193003/diff/180001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:1456: // orr     Src0RHi, T, Src0RHi, lsl Src1RLo
On 2015/09/24 05:57:13, stichnot wrote:
> I don't think this is a correct abstraction of what llc does, since it's
> unlikely to smash Src0RHi and Src0RLo if Src0 is live beyond the shl
> instruction.  Maybe it should look more like this?
> 
>   mov t_b.lo, b.lo
>   mov t_b.hi, b.hi
>   mov t_c.lo, c.lo
>   rsb T, t_c.lo, #32
>   lsr T1, t_b.lo, T
>   orr T3, T1, t_b.hi, lsl t_c.lo
>   sub T2, t_c.lo, #32
>   cmp T2, #0
>   lslge T3, t_b.lo, T2
>   lsl T5, t_b.lo, t_c.lo

Done.

https://codereview.chromium.org/1359193003/diff/180001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:1483: Variable *T1 = makeReg(IceType_i32);
On 2015/09/24 05:57:13, stichnot wrote:
> Suggestion: I think the pseudo-assembly reads easier if you move the
makeReg()s
> and getConstant()s to the top.  (Then it will be more clear that T4 is
missing,
> though not so clear that T3 is defined before T2...)

Done.

https://codereview.chromium.org/1359193003/diff/180001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:1571: Variable *T = makeReg(Dest->getType());
On 2015/09/24 05:57:13, stichnot wrote:
> What do you think about bundling these 4 statements into a macro or something
> like that?

I thought about that, but these are not going to be here very long. I'll leave
them as is.

https://codereview.chromium.org/1359193003/diff/180001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:2354:
UnimplementedError(Func->getContext()->getFlags());
On 2015/09/24 05:57:13, stichnot wrote:
> Usually the UnimplementedError() comes after the placeholder lowering?

Done.

https://codereview.chromium.org/1359193003/diff/180001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:2990: _mov(Reg, Src);
On 2015/09/24 05:57:13, stichnot wrote:
> Should there be just one _mov() ?

Done.

https://codereview.chromium.org/1359193003/diff/180001/src/IceTargetLoweringA...
File src/IceTargetLoweringARM32.h (right):

https://codereview.chromium.org/1359193003/diff/180001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.h:249: /// _ldr, for all your Variable to memory data
moves. It handles all types
On 2015/09/24 05:57:14, stichnot wrote:
> I would describe this as "memory to Variable data moves", right?
> 
> Essentially same comment for _str below.

Done.

https://codereview.chromium.org/1359193003/diff/180001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.h:277: /// If Dest=nullptr is passed in, then a new
variable
On 2015/09/24 05:57:13, stichnot wrote:
> Delete this part of the comment?  (contradicted below)

Done.

https://codereview.chromium.org/1359193003/diff/180001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.h:283: // automagically of Dest was nullptr. It won't
do that anymore, so we keep
On 2015/09/24 05:57:13, stichnot wrote:
> s/of/if/

Done.

[John, 2015-09-24 21:28:40]

https://codereview.chromium.org/1359193003/diff/180001/crosstest/test_arith_m...
File crosstest/test_arith_main.cpp (right):

https://codereview.chromium.org/1359193003/diff/180001/crosstest/test_arith_m...
crosstest/test_arith_main.cpp:157: #ifndef ARM32
On 2015/09/24 05:57:13, stichnot wrote:
> I'd like to see TODOs on all these "#ifndef ARM32" lines just to be clear.

Done.

https://codereview.chromium.org/1359193003/diff/180001/crosstest/test_calling...
File crosstest/test_calling_conv.h (right):

https://codereview.chromium.org/1359193003/diff/180001/crosstest/test_calling...
crosstest/test_calling_conv.h:35: typedef void(callee_vlvlivfvdviv_Ty)(v4f32,
int64, v4f32, int, int64, v4f32,
On 2015/09/24 05:57:13, stichnot wrote:
> I'm not sure why you needed to change the argument order, but now the name of
> the function type doesn't match the arguments.
> 
> Old: callee_vlvlivfvdviv_Ty
> New: callee_vlvilvfvdviv_Ty
>                ^^
Done.

For reference: The two int64 parameters fit nicely in r0-r1, and r2-r3. By
swapping the arguments I can test that the ARM32 target is passing the second
int64 in the stack.

https://codereview.chromium.org/1359193003/diff/180001/pydir/crosstest.py
File pydir/crosstest.py (left):

https://codereview.chromium.org/1359193003/diff/180001/pydir/crosstest.py#old...
pydir/crosstest.py:38: '--test-stack-extra', '4084']
On 2015/09/24 05:57:13, stichnot wrote:
> This seems useful for full testing, so why remove it?

Because the lowering is broken. I added a todo to revert this.

https://codereview.chromium.org/1359193003/diff/180001/src/IceInstARM32.cpp
File src/IceInstARM32.cpp (right):

https://codereview.chromium.org/1359193003/diff/180001/src/IceInstARM32.cpp#n...
src/IceInstARM32.cpp:214: /*if (isScalarFloatingType(Ty))
On 2015/09/24 05:57:13, stichnot wrote:
> Explain or delete comment?

Done.

https://codereview.chromium.org/1359193003/diff/180001/src/IceInstARM32.h
File src/IceInstARM32.h (right):

https://codereview.chromium.org/1359193003/diff/180001/src/IceInstARM32.h#new...
src/IceInstARM32.h:785: //!!!!!!!!!!!!!!!!!!!!!!using InstARM32Mov =
InstARM32Movlike<InstARM32::Mov>;
On 2015/09/24 05:57:13, stichnot wrote:
> Explain this comment, or delete it?

Done.

https://codereview.chromium.org/1359193003/diff/180001/src/IceTargetLoweringA...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1359193003/diff/180001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:612: // fp types, cannot have an index register.)
This ARM nonsense makes one
On 2015/09/24 05:57:13, stichnot wrote:
> I don't think that sort of editorial comment should be in this code base...

That's because you are not working with ARM32.. :P

Just joking. You're right. Done.

https://codereview.chromium.org/1359193003/diff/180001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:1456: // orr     Src0RHi, T, Src0RHi, lsl Src1RLo
On 2015/09/24 05:57:13, stichnot wrote:
> I don't think this is a correct abstraction of what llc does, since it's
> unlikely to smash Src0RHi and Src0RLo if Src0 is live beyond the shl
> instruction.  Maybe it should look more like this?
> 
>   mov t_b.lo, b.lo
>   mov t_b.hi, b.hi
>   mov t_c.lo, c.lo
>   rsb T, t_c.lo, #32
>   lsr T1, t_b.lo, T
>   orr T3, T1, t_b.hi, lsl t_c.lo
>   sub T2, t_c.lo, #32
>   cmp T2, #0
>   lslge T3, t_b.lo, T2
>   lsl T5, t_b.lo, t_c.lo

Done.

https://codereview.chromium.org/1359193003/diff/180001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:1483: Variable *T1 = makeReg(IceType_i32);
On 2015/09/24 05:57:13, stichnot wrote:
> Suggestion: I think the pseudo-assembly reads easier if you move the
makeReg()s
> and getConstant()s to the top.  (Then it will be more clear that T4 is
missing,
> though not so clear that T3 is defined before T2...)

Done.

https://codereview.chromium.org/1359193003/diff/180001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:1571: Variable *T = makeReg(Dest->getType());
On 2015/09/24 05:57:13, stichnot wrote:
> What do you think about bundling these 4 statements into a macro or something
> like that?

I thought about that, but these are not going to be here very long. I'll leave
them as is.

https://codereview.chromium.org/1359193003/diff/180001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:2354:
UnimplementedError(Func->getContext()->getFlags());
On 2015/09/24 05:57:13, stichnot wrote:
> Usually the UnimplementedError() comes after the placeholder lowering?

Done.

https://codereview.chromium.org/1359193003/diff/180001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:2990: _mov(Reg, Src);
On 2015/09/24 05:57:13, stichnot wrote:
> Should there be just one _mov() ?

Done.

https://codereview.chromium.org/1359193003/diff/180001/src/IceTargetLoweringA...
File src/IceTargetLoweringARM32.h (right):

https://codereview.chromium.org/1359193003/diff/180001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.h:249: /// _ldr, for all your Variable to memory data
moves. It handles all types
On 2015/09/24 05:57:14, stichnot wrote:
> I would describe this as "memory to Variable data moves", right?
> 
> Essentially same comment for _str below.

Done.

https://codereview.chromium.org/1359193003/diff/180001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.h:277: /// If Dest=nullptr is passed in, then a new
variable
On 2015/09/24 05:57:13, stichnot wrote:
> Delete this part of the comment?  (contradicted below)

Done.

https://codereview.chromium.org/1359193003/diff/180001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.h:283: // automagically of Dest was nullptr. It won't
do that anymore, so we keep
On 2015/09/24 05:57:13, stichnot wrote:
> s/of/if/

Done.

[John, 2015-09-25 09:57:54]

Well, I could resist figuring out what was wrong with the stack slot
legalization :)

I reset the --test-stack-extra to 4084, which now works.

Also a minor bugfix for textual operand emission.

[Jim Stichnoth, 2015-09-25 21:24:19]

otherwise lgtm

https://codereview.chromium.org/1359193003/diff/260001/src/IceTargetLoweringA...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1359193003/diff/260001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:445: // turns out that the lowering
FakeDefs/FakeUses such a variables. If they end
I think some words are missing from this sentence?

https://codereview.chromium.org/1359193003/diff/260001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:446: // up being assigned an illegal offset we
get a runtime error. We liberaly
liberally

https://codereview.chromium.org/1359193003/diff/260001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:1540: case InstArithmetic::Ashr: {
Do you think there's an elegant way to move "case Lshr:" and "case Ashr:" closer
together?  I spent way too long trying to figure out why this patchset changed
only 2 out of the 3 shift lowering sequences. :)

[John, 2015-09-25 22:18:29]

https://codereview.chromium.org/1359193003/diff/260001/src/IceTargetLoweringA...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1359193003/diff/260001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:446: // up being assigned an illegal offset we
get a runtime error. We liberaly
On 2015/09/25 21:24:19, stichnot wrote:
> liberally

Done.

https://codereview.chromium.org/1359193003/diff/260001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:1540: case InstArithmetic::Ashr: {
On 2015/09/25 21:24:19, stichnot wrote:
> Do you think there's an elegant way to move "case Lshr:" and "case Ashr:"
closer
> together?  I spent way too long trying to figure out why this patchset changed
> only 2 out of the 3 shift lowering sequences. :)

I tried my best. :)

[John, 2015-09-25 22:19:49]

Committed patchset #16 (id:280001) manually as
ba6a67c960a126d85b94a87c8ea7358bec0da048 (tree was closed).