Lower the rest of the vector arithmetic operations.

Lower the rest of the vector arithmetic operations.

The instructions emitted by the lowering operations require memory
operands to be aligned to 16 bytes. Since there is no support for
aligning memory operands in Subzero, do the arithmetic in registers for
now.

Add vector arithmetic to the arith crosstest. Pass the -mstackrealign
parameter to the crosstest clang so that llc code called back from
Subzero code (helper calls) doesn't assume that the stack is aligned at
the entry to the call.

BUG=none
R=jvoung@chromium.org, stichnot@chromium.org

Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=7fa22d8

[Jim Stichnoth, 2014-07-16 19:17:11]

lgtm with nits

https://codereview.chromium.org/397833002/diff/20001/crosstest/crosstest.py
File crosstest/crosstest.py (right):

https://codereview.chromium.org/397833002/diff/20001/crosstest/crosstest.py#n...
crosstest/crosstest.py:133: shellcmd([os.path.join(llvm_bin_path, linker), '-g',
'-m32', '-mstackrealign',
80-col

https://codereview.chromium.org/397833002/diff/20001/crosstest/test_arith_mai...
File crosstest/test_arith_main.cpp (right):

https://codereview.chromium.org/397833002/diff/20001/crosstest/test_arith_mai...
crosstest/test_arith_main.cpp:127: // TODO: Replace with a portable PRNG from
C++11.
It would be good to have a comment explaining that vectors are randomly but
deterministically constructed from a pool of interesting scalar values because
testing all possible combinations of scalar values would take too long.  (People
tend to have knee-jerk suspicious reactions to randomized tests like this.)

BTW, complete testing should be tractable for 4-element vectors, at least with
the current scalar pool sizes.

https://codereview.chromium.org/397833002/diff/20001/src/IceTargetLoweringX86...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/397833002/diff/20001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:93: IceString typeIdentString(const Type Ty) {
Put inside an anonymous namespace

https://codereview.chromium.org/397833002/diff/20001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1157: // implemented, replace legalizeToVar(Src1)
with Src1.
I'm not adamant about this, but I think it might be easier for the person who
eventually fixes this TODO if you did something like:

#define LEGAL_HACK(s) legalizeToVar((s))
  ... LEGAL_HACK(Src1) ...

and then advise removing LEGAL_HACK().

https://codereview.chromium.org/397833002/diff/20001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1159: // TODO: Trap on divide and modulo by zero.
I believe this should only trap for integer div/mod, not floating point. 
Hopefully the issue tracker will clarify...

[jvoung (off chromium), 2014-07-16 19:23:26]

https://codereview.chromium.org/397833002/diff/20001/crosstest/crosstest.py
File crosstest/crosstest.py (right):

https://codereview.chromium.org/397833002/diff/20001/crosstest/crosstest.py#n...
crosstest/crosstest.py:133: shellcmd([os.path.join(llvm_bin_path, linker), '-g',
'-m32', '-mstackrealign',
Add a TODO to remove mstackrealign.

https://codereview.chromium.org/397833002/diff/20001/crosstest/test_arith_mai...
File crosstest/test_arith_main.cpp (right):

https://codereview.chromium.org/397833002/diff/20001/crosstest/test_arith_mai...
crosstest/test_arith_main.cpp:179: SINTOP_TABLE
indent seems a bit more than usual, for SINTOP_TABLE

https://codereview.chromium.org/397833002/diff/20001/src/IceTargetLoweringX86...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/397833002/diff/20001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1199: // pshufd  T2, Src0, [1,0,3,0]
nit: The ordering of the vector ([1, 0, 3, 0]) is a bit unclear at first glance.
I'm assuming you have index 0 on the left and index 3 on the right. It might be
more clear if it was the other way around, to match the order of bits of the
shuffle mask?

[wala, 2014-07-17 01:34:54]

https://codereview.chromium.org/397833002/diff/20001/crosstest/crosstest.py
File crosstest/crosstest.py (right):

https://codereview.chromium.org/397833002/diff/20001/crosstest/crosstest.py#n...
crosstest/crosstest.py:133: shellcmd([os.path.join(llvm_bin_path, linker), '-g',
'-m32', '-mstackrealign',
On 2014/07/16 19:17:10, stichnot wrote:
> 80-col

Done.

https://codereview.chromium.org/397833002/diff/20001/crosstest/crosstest.py#n...
crosstest/crosstest.py:133: shellcmd([os.path.join(llvm_bin_path, linker), '-g',
'-m32', '-mstackrealign',
On 2014/07/16 19:23:25, jvoung wrote:
> Add a TODO to remove mstackrealign.

Done.

https://codereview.chromium.org/397833002/diff/20001/crosstest/test_arith_mai...
File crosstest/test_arith_main.cpp (right):

https://codereview.chromium.org/397833002/diff/20001/crosstest/test_arith_mai...
crosstest/test_arith_main.cpp:127: // TODO: Replace with a portable PRNG from
C++11.
On 2014/07/16 19:17:10, stichnot wrote:
> It would be good to have a comment explaining that vectors are randomly but
> deterministically constructed from a pool of interesting scalar values because
> testing all possible combinations of scalar values would take too long. 
(People
> tend to have knee-jerk suspicious reactions to randomized tests like this.)
> 
> BTW, complete testing should be tractable for 4-element vectors, at least with
> the current scalar pool sizes.

Adding a comment: Done.

The current size of the integer table is 24. The number of pairs to test from
the integer table would be (24^4)^2 ~ 10^11, which is not quite tractable. It
would overflow TotalTests :)

https://codereview.chromium.org/397833002/diff/20001/crosstest/test_arith_mai...
crosstest/test_arith_main.cpp:179: SINTOP_TABLE
On 2014/07/16 19:23:26, jvoung wrote:
> indent seems a bit more than usual, for SINTOP_TABLE

Done.

https://codereview.chromium.org/397833002/diff/20001/src/IceTargetLoweringX86...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/397833002/diff/20001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:93: IceString typeIdentString(const Type Ty) {
On 2014/07/16 19:17:10, stichnot wrote:
> Put inside an anonymous namespace

This is already inside an anonymous namespace.

https://codereview.chromium.org/397833002/diff/20001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1157: // implemented, replace legalizeToVar(Src1)
with Src1.
On 2014/07/16 19:17:10, stichnot wrote:
> I'm not adamant about this, but I think it might be easier for the person who
> eventually fixes this TODO if you did something like:
> 
> #define LEGAL_HACK(s) legalizeToVar((s))
>   ... LEGAL_HACK(Src1) ...
> 
> and then advise removing LEGAL_HACK().

Good idea. Done.

https://codereview.chromium.org/397833002/diff/20001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1159: // TODO: Trap on divide and modulo by zero.
On 2014/07/16 19:17:10, stichnot wrote:
> I believe this should only trap for integer div/mod, not floating point. 
> Hopefully the issue tracker will clarify...

Done.

https://codereview.chromium.org/397833002/diff/20001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1199: // pshufd  T2, Src0, [1,0,3,0]
On 2014/07/16 19:23:26, jvoung wrote:
> nit: The ordering of the vector ([1, 0, 3, 0]) is a bit unclear at first
glance.
> I'm assuming you have index 0 on the left and index 3 on the right. It might
be
> more clear if it was the other way around, to match the order of bits of the
> shuffle mask?

I write vectors as if they were arrays in the comments. The representation of
arrays starts with index 0 on the left and I don't want to switch orderings too
much in the pseudocode.

Furthermore, the Src[...] syntax that I use in the comments below this line also
starts with index 0 on the left.

These are the changes that I made: I changed the [ and ] to { and } to emphasize
that I'm treating the values as if they were arrays and starting with index 0 on
the left. I also added a comment that index 0 starts on the left.

If that's not alright, I can change it to go from right to left.

[Jim Stichnoth, 2014-07-17 13:03:13]

https://codereview.chromium.org/397833002/diff/20001/crosstest/test_arith_mai...
File crosstest/test_arith_main.cpp (right):

https://codereview.chromium.org/397833002/diff/20001/crosstest/test_arith_mai...
crosstest/test_arith_main.cpp:127: // TODO: Replace with a portable PRNG from
C++11.
On 2014/07/17 01:34:53, wala wrote:
> On 2014/07/16 19:17:10, stichnot wrote:
> > It would be good to have a comment explaining that vectors are randomly but
> > deterministically constructed from a pool of interesting scalar values
because
> > testing all possible combinations of scalar values would take too long. 
> (People
> > tend to have knee-jerk suspicious reactions to randomized tests like this.)
> > 
> > BTW, complete testing should be tractable for 4-element vectors, at least
with
> > the current scalar pool sizes.
> 
> Adding a comment: Done.
> 
> The current size of the integer table is 24. The number of pairs to test from
> the integer table would be (24^4)^2 ~ 10^11, which is not quite tractable. It
> would overflow TotalTests :)

Ah yes - I was forgetting that extra ^2 in my calculation. :)

https://codereview.chromium.org/397833002/diff/20001/src/IceTargetLoweringX86...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/397833002/diff/20001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:93: IceString typeIdentString(const Type Ty) {
On 2014/07/17 01:34:53, wala wrote:
> On 2014/07/16 19:17:10, stichnot wrote:
> > Put inside an anonymous namespace
> 
> This is already inside an anonymous namespace.

D'oh!  Sorry!

[jvoung (off chromium), 2014-07-17 15:00:39]

lgtm

https://codereview.chromium.org/397833002/diff/20001/src/IceTargetLoweringX86...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/397833002/diff/20001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1199: // pshufd  T2, Src0, [1,0,3,0]
On 2014/07/17 01:34:53, wala wrote:
> On 2014/07/16 19:23:26, jvoung wrote:
> > nit: The ordering of the vector ([1, 0, 3, 0]) is a bit unclear at first
> glance.
> > I'm assuming you have index 0 on the left and index 3 on the right. It might
> be
> > more clear if it was the other way around, to match the order of bits of the
> > shuffle mask?
> 
> I write vectors as if they were arrays in the comments. The representation of
> arrays starts with index 0 on the left and I don't want to switch orderings
too
> much in the pseudocode.
> 
> Furthermore, the Src[...] syntax that I use in the comments below this line
also
> starts with index 0 on the left.
> 
> These are the changes that I made: I changed the [ and ] to { and } to
emphasize
> that I'm treating the values as if they were arrays and starting with index 0
on
> the left. I also added a comment that index 0 starts on the left.
> 
> If that's not alright, I can change it to go from right to left.

Okay, that helps some.

[wala, 2014-07-17 19:41:34]

Committed patchset #4 manually as r7fa22d8 (presubmit successful).