Subzero: Fix sext/zext lowering with i1 source operands.

Subzero: Fix sext/zext lowering with i1 source operands.

Also declare a few variables as Constant* instead of Operand* when they hold the result of Ctx->getConstantInt(), to be consistent with the rest of the code.

BUG= none
R=jvoung@chromium.org

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

[Jim Stichnoth, 2014-09-04 20:00:08]

stichnot@chromium.org changed reviewers:
+ jvoung@chromium.org

[jvoung (off chromium), 2014-09-04 21:05:41]

https://codereview.chromium.org/541093002/diff/1/src/IceTargetLoweringX8632.cpp
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/541093002/diff/1/src/IceTargetLoweringX8632.c...
src/IceTargetLoweringX8632.cpp:1927: Constant *ShiftAmount =
Ctx->getConstantInt(IceType_i32, 31);
nit: Since the later Shift is always needed, could have moved that earlier and
used that. Either way.

https://codereview.chromium.org/541093002/diff/1/src/IceTargetLoweringX8632.c...
src/IceTargetLoweringX8632.cpp:1970: size_t DestBits = CHAR_BIT *
typeWidthInBytes(Dest->getType());
Should this be X86_CHAR_BIT instead (which the vector ShiftAmount uses above)?

https://codereview.chromium.org/541093002/diff/1/src/IceTargetLoweringX8632.c...
src/IceTargetLoweringX8632.cpp:2020: // t1 = movzx src; dst = t1
Maybe move the t1 = movzx comment to the } else {

or add the "- or -" clause

https://codereview.chromium.org/541093002/diff/1/src/IceTargetLoweringX8632.c...
src/IceTargetLoweringX8632.cpp:2022: if (Src0RM->getType() == IceType_i1) {
Is this case possible? Or is it already caught by the earlier:

 } else if (Src0RM->getType() == IceType_i1) {

?

Seems like it would be better to remove the earlier check to be consistent with
the sext structure.

https://codereview.chromium.org/541093002/diff/1/src/IceTargetLoweringX8632.c...
src/IceTargetLoweringX8632.cpp:3953: Constant *Value =
Ctx->getConstantInt(IceType_i32, Inst->getValue(I));
This seems separate. Add a comment in the commit message?

https://codereview.chromium.org/541093002/diff/1/tests_lit/llvm2ice_tests/64b...
File tests_lit/llvm2ice_tests/64bit.pnacl.ll (right):

https://codereview.chromium.org/541093002/diff/1/tests_lit/llvm2ice_tests/64b...
tests_lit/llvm2ice_tests/64bit.pnacl.ll:719: ; CHECK: and
Could check that the and mask is "1" also -- but the other test does that check
too.

[Jim Stichnoth, 2014-09-04 21:43:07]

https://codereview.chromium.org/541093002/diff/1/src/IceTargetLoweringX8632.cpp
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/541093002/diff/1/src/IceTargetLoweringX8632.c...
src/IceTargetLoweringX8632.cpp:1927: Constant *ShiftAmount =
Ctx->getConstantInt(IceType_i32, 31);
On 2014/09/04 21:05:41, jvoung wrote:
> nit: Since the later Shift is always needed, could have moved that earlier and
> used that. Either way.

Done.

https://codereview.chromium.org/541093002/diff/1/src/IceTargetLoweringX8632.c...
src/IceTargetLoweringX8632.cpp:1970: size_t DestBits = CHAR_BIT *
typeWidthInBytes(Dest->getType());
On 2014/09/04 21:05:41, jvoung wrote:
> Should this be X86_CHAR_BIT instead (which the vector ShiftAmount uses above)?

Done.

https://codereview.chromium.org/541093002/diff/1/src/IceTargetLoweringX8632.c...
src/IceTargetLoweringX8632.cpp:2020: // t1 = movzx src; dst = t1
On 2014/09/04 21:05:41, jvoung wrote:
> Maybe move the t1 = movzx comment to the } else {
> 
> or add the "- or -" clause

Done (as part of the next comment below).

https://codereview.chromium.org/541093002/diff/1/src/IceTargetLoweringX8632.c...
src/IceTargetLoweringX8632.cpp:2022: if (Src0RM->getType() == IceType_i1) {
On 2014/09/04 21:05:41, jvoung wrote:
> Is this case possible? Or is it already caught by the earlier:
> 
>  } else if (Src0RM->getType() == IceType_i1) {
> 
> ?
> 
> Seems like it would be better to remove the earlier check to be consistent
with
> the sext structure.

Oops, somehow I thought the i1 case wasn't being properly handled.

I've changed both sext and zext to be the typical form where vector is checked
first, followed by i64, followed by others.

https://codereview.chromium.org/541093002/diff/1/src/IceTargetLoweringX8632.c...
src/IceTargetLoweringX8632.cpp:3953: Constant *Value =
Ctx->getConstantInt(IceType_i32, Inst->getValue(I));
On 2014/09/04 21:05:41, jvoung wrote:
> This seems separate. Add a comment in the commit message?

Done.

https://codereview.chromium.org/541093002/diff/1/tests_lit/llvm2ice_tests/64b...
File tests_lit/llvm2ice_tests/64bit.pnacl.ll (right):

https://codereview.chromium.org/541093002/diff/1/tests_lit/llvm2ice_tests/64b...
tests_lit/llvm2ice_tests/64bit.pnacl.ll:719: ; CHECK: and
On 2014/09/04 21:05:41, jvoung wrote:
> Could check that the and mask is "1" also -- but the other test does that
check
> too.

Done.

[jvoung (off chromium), 2014-09-04 23:30:25]

lgtm

[Jim Stichnoth, 2014-09-04 23:39:05]

Committed patchset #2 (id:20001) manually as dd30c81 (tree was closed).