Lower icmp operations between vector values.

Lower icmp operations between vector values.

SSE2 only has signed integer comparison. Unsigned compares are
implemented by inverting the sign bits of the operands and doing a
signed compare.

A common pattern in clang generated IR is a vector compare which
generates an i1 vector followed by a sign extension of the result of the
compare. The x86 comparison instructions already generate sign extended
values, so we can eliminate unnecessary sext operations that follow
compares in the IR.

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=9a0168a

[Jim Stichnoth, 2014-07-23 17:29:29]

Looking good, apart from my low-level comment.

https://codereview.chromium.org/412593002/diff/80001/src/IceInst.def
File src/IceInst.def (right):

https://codereview.chromium.org/412593002/diff/80001/src/IceInst.def#newcode86
src/IceInst.def:86: //#define X(tag, str)
Update this comment

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

https://codereview.chromium.org/412593002/diff/80001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:2295: if (Inst->isUnsigned()) {
I have a minor problem and a major problem with isUnsigned().

Minor problem.  In IceInst.def, I would one of 3 values to describe the
signed-ness of the operation: -1 for definitely signed (sge, sgt, sle, slt), 1
for definitely unsigned (uge, ugt, ult, ule), and 0 for "don't care" or
non-signed (eq, ne).

Major problem.  I don't think the machinery behind isUnsigned() is justified for
this single use.  Why not just add one more switch statement on
Inst->getCondition() here?  (My opinion would change if we developed more use
cases for this operation.)

[jvoung (off chromium), 2014-07-23 18:54:55]

mostly nits from me

https://codereview.chromium.org/412593002/diff/80001/crosstest/test_icmp_main...
File crosstest/test_icmp_main.cpp (right):

https://codereview.chromium.org/412593002/diff/80001/crosstest/test_icmp_main...
crosstest/test_icmp_main.cpp:185: // True on wraparound
nit: "Return true"

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

https://codereview.chromium.org/412593002/diff/80001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1282: // Sz_shl_v4i32, Sz_shl_v8i16, Sz_shl_v16i8
Could this have used _psll then, for i32 and i16 elements?

https://codereview.chromium.org/412593002/diff/80001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:2374: }
could this just return;

and then the scalar version doesn't need to be indented?

[jvoung (off chromium), 2014-07-23 19:02:12]

https://codereview.chromium.org/412593002/diff/80001/tests_lit/llvm2ice_tests...
File tests_lit/llvm2ice_tests/vector-icmp.ll (right):

https://codereview.chromium.org/412593002/diff/80001/tests_lit/llvm2ice_tests...
tests_lit/llvm2ice_tests/vector-icmp.ll:4: ; RUN: %llvm2ice -O2 --verbose none
%s | FileCheck %s
Might be good to have a test that the sext peephole optimization works too.

[wala, 2014-07-23 20:40:37]

https://codereview.chromium.org/412593002/diff/80001/crosstest/test_icmp_main...
File crosstest/test_icmp_main.cpp (right):

https://codereview.chromium.org/412593002/diff/80001/crosstest/test_icmp_main...
crosstest/test_icmp_main.cpp:185: // True on wraparound
On 2014/07/23 18:54:55, jvoung wrote:
> nit: "Return true"

Done.

https://codereview.chromium.org/412593002/diff/80001/src/IceInst.def
File src/IceInst.def (right):

https://codereview.chromium.org/412593002/diff/80001/src/IceInst.def#newcode86
src/IceInst.def:86: //#define X(tag, str)
On 2014/07/23 17:29:28, stichnot wrote:
> Update this comment

Reverted to older version.

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

https://codereview.chromium.org/412593002/diff/80001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1282: // Sz_shl_v4i32, Sz_shl_v8i16, Sz_shl_v16i8
On 2014/07/23 18:54:55, jvoung wrote:
> Could this have used _psll then, for i32 and i16 elements?

No. Unfortunately _psll shifts all the fields in the vector by the same amount.

The AVX2 instruction set has vpsllvd and vpsllvw that can be used for the
purposes of lowering shl.

https://codereview.chromium.org/412593002/diff/80001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:2295: if (Inst->isUnsigned()) {
On 2014/07/23 17:29:28, stichnot wrote:
> I have a minor problem and a major problem with isUnsigned().
> 
> Minor problem.  In IceInst.def, I would one of 3 values to describe the
> signed-ness of the operation: -1 for definitely signed (sge, sgt, sle, slt), 1
> for definitely unsigned (uge, ugt, ult, ule), and 0 for "don't care" or
> non-signed (eq, ne).
> 
> Major problem.  I don't think the machinery behind isUnsigned() is justified
for
> this single use.  Why not just add one more switch statement on
> Inst->getCondition() here?  (My opinion would change if we developed more use
> cases for this operation.)

Done.

Removed the unsigned attribute.

https://codereview.chromium.org/412593002/diff/80001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:2374: }
On 2014/07/23 18:54:55, jvoung wrote:
> could this just return;
> 
> and then the scalar version doesn't need to be indented?

Done.

https://codereview.chromium.org/412593002/diff/80001/tests_lit/llvm2ice_tests...
File tests_lit/llvm2ice_tests/vector-icmp.ll (right):

https://codereview.chromium.org/412593002/diff/80001/tests_lit/llvm2ice_tests...
tests_lit/llvm2ice_tests/vector-icmp.ll:4: ; RUN: %llvm2ice -O2 --verbose none
%s | FileCheck %s
On 2014/07/23 19:02:12, jvoung wrote:
> Might be good to have a test that the sext peephole optimization works too.

Done.

[Jim Stichnoth, 2014-07-23 20:58:02]

lgtm

https://codereview.chromium.org/412593002/diff/100001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/412593002/diff/100001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.cpp:2398: // i64.
Could revert this whitespace-only change. :)

[wala, 2014-07-23 21:02:22]

https://codereview.chromium.org/412593002/diff/100001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/412593002/diff/100001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.cpp:2398: // i64.
On 2014/07/23 20:58:01, stichnot wrote:
> Could revert this whitespace-only change. :)

Done.

[jvoung (off chromium), 2014-07-23 21:46:49]

lgtm

[wala, 2014-07-23 21:56:14]

Committed patchset #9 manually as r9a0168a (presubmit successful).