[SubZero] Implement Fcmp, ICmp, Cast and Select for vector type

[SubZero] Implement Fcmp, ICmp, Cast and Select for vector type

The patch scalarizes Fcmp, ICmp, Cast and Select for operands of vector type.

R=stichnot@chromium.org

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

[jaydeep.patil, 2016-10-12 07:44:03]

Description was changed from

==========
[SubZero] Implement Fcmp, ICmp, Cast and Select for vector type

The patch scalarizes  Fcmp, ICmp, Cast and Select for operands of vector type.
==========

to

==========
[SubZero] Implement Fcmp, ICmp, Cast and Select for vector type

The patch scalarizes Fcmp, ICmp, Cast and Select for operands of vector type.
==========

[jaydeep.patil, 2016-10-12 07:44:03]

jaydeep.patil@imgtec.com changed reviewers:
+ eholk@chromium.org, jpp@chromium.org, kschimpf@google.com,
makdstefan@gmail.com, sagar.thakur@imgtec.com, stichnot@chromium.org

[Jim Stichnoth, 2016-10-14 13:43:37]

Could you also rebase to latest master?  I wanted to test this out locally, but
I wasn't 100% sure how to resolve the merge conflict.

https://codereview.chromium.org/2412053002/diff/1/src/IceTargetLoweringMIPS32...
File src/IceTargetLoweringMIPS32.cpp (right):

https://codereview.chromium.org/2412053002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:222: 
Would be nice to factor out Dest queries to the top here, something like:

Variable *Dest = Instr->getDest();
const Type DestTy = Dest ? Dest->getType() : IceType_void;

https://codereview.chromium.org/2412053002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:235: auto *VarVecOn32 =
llvm::dyn_cast<VariableVecOn32>(T);
Use llvm::cast<> since the next statement assumes the result is non-nullptr.

https://codereview.chromium.org/2412053002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:241: auto *OpT =
Func->makeVariable(typeElementType(T->getType()));
I would use DestTy instead of T->getType() just to make it a bit more clear.

https://codereview.chromium.org/2412053002/diff/1/tests_lit/llvm2ice_tests/ve...
File tests_lit/llvm2ice_tests/vector-icmp.ll (right):

https://codereview.chromium.org/2412053002/diff/1/tests_lit/llvm2ice_tests/ve...
tests_lit/llvm2ice_tests/vector-icmp.ll:14: ; instruction is alrady sign
extended.  Sign extension to 4 x i32 uses
Could you fix this typo while you're at it?  :)
s/alrady/already/

https://codereview.chromium.org/2412053002/diff/1/tests_lit/llvm2ice_tests/ve...
tests_lit/llvm2ice_tests/vector-icmp.ll:15: ; the pslld instruction.
maybe "uses the pslld instruction on x86" ?

https://codereview.chromium.org/2412053002/diff/1/tests_lit/llvm2ice_tests/ve...
tests_lit/llvm2ice_tests/vector-icmp.ll:25: ; MIPS32-LABEL:
test_sext_elimination
Regarding this file: WOW!

These MIPS tests are highly specific and I hope the lowering never needs to
change...

[jaydeep.patil, 2016-10-15 03:47:57]

https://codereview.chromium.org/2412053002/diff/1/src/IceTargetLoweringMIPS32...
File src/IceTargetLoweringMIPS32.cpp (right):

https://codereview.chromium.org/2412053002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:222: 
On 2016/10/14 13:43:37, Jim Stichnoth wrote:
> Would be nice to factor out Dest queries to the top here, something like:
> 
> Variable *Dest = Instr->getDest();
> const Type DestTy = Dest ? Dest->getType() : IceType_void;

Done.

https://codereview.chromium.org/2412053002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:235: auto *VarVecOn32 =
llvm::dyn_cast<VariableVecOn32>(T);
On 2016/10/14 13:43:37, Jim Stichnoth wrote:
> Use llvm::cast<> since the next statement assumes the result is non-nullptr.

Done.

https://codereview.chromium.org/2412053002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:241: auto *OpT =
Func->makeVariable(typeElementType(T->getType()));
On 2016/10/14 13:43:37, Jim Stichnoth wrote:
> I would use DestTy instead of T->getType() just to make it a bit more clear.

Done.

https://codereview.chromium.org/2412053002/diff/1/tests_lit/llvm2ice_tests/ve...
File tests_lit/llvm2ice_tests/vector-icmp.ll (right):

https://codereview.chromium.org/2412053002/diff/1/tests_lit/llvm2ice_tests/ve...
tests_lit/llvm2ice_tests/vector-icmp.ll:14: ; instruction is alrady sign
extended.  Sign extension to 4 x i32 uses
On 2016/10/14 13:43:37, Jim Stichnoth wrote:
> Could you fix this typo while you're at it?  :)
> s/alrady/already/

Done.

https://codereview.chromium.org/2412053002/diff/1/tests_lit/llvm2ice_tests/ve...
tests_lit/llvm2ice_tests/vector-icmp.ll:15: ; the pslld instruction.
On 2016/10/14 13:43:37, Jim Stichnoth wrote:
> maybe "uses the pslld instruction on x86" ?

Done.

https://codereview.chromium.org/2412053002/diff/1/tests_lit/llvm2ice_tests/ve...
tests_lit/llvm2ice_tests/vector-icmp.ll:25: ; MIPS32-LABEL:
test_sext_elimination
On 2016/10/14 13:43:37, Jim Stichnoth wrote:
> Regarding this file: WOW!
> 
> These MIPS tests are highly specific and I hope the lowering never needs to
> change...

Acknowledged.

[Jim Stichnoth, 2016-10-17 13:33:35]

lgtm

[Jim Stichnoth, 2016-10-17 13:33:52]

Description was changed from

==========
[SubZero] Implement Fcmp, ICmp, Cast and Select for vector type

The patch scalarizes Fcmp, ICmp, Cast and Select for operands of vector type.
==========

to

==========
[SubZero] Implement Fcmp, ICmp, Cast and Select for vector type

The patch scalarizes Fcmp, ICmp, Cast and Select for operands of vector type.

R=stichnot@chromium.org

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

[Jim Stichnoth, 2016-10-17 13:33:54]

Committed patchset #2 (id:20001) manually as
3a01f337a848f4200542393d60d32c7e79ff42ea (presubmit successful).