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

src/IceTargetLoweringX8632.cpp

Index: src/IceTargetLoweringX8632.cpp
diff --git a/src/IceTargetLoweringX8632.cpp b/src/IceTargetLoweringX8632.cpp
index e23e5ade6a62db1598c8e2c5e7eeaac6f6daa099..aaa39d3a72e5582dce421986e2a2b35c98f6dc78 100644
--- a/src/IceTargetLoweringX8632.cpp
+++ b/src/IceTargetLoweringX8632.cpp
@@ -1921,10 +1921,16 @@ void TargetX8632::lowerCast(const InstCast *Inst) {
       Variable *DestLo = llvm::cast<Variable>(loOperand(Dest));
       Variable *DestHi = llvm::cast<Variable>(hiOperand(Dest));
       Variable *T_Lo = makeReg(DestLo->getType());
-      if (Src0RM->getType() == IceType_i32)
+      if (Src0RM->getType() == IceType_i32) {
         _mov(T_Lo, Src0RM);
-      else
+      } else if (Src0RM->getType() == IceType_i1) {
+        Constant *ShiftAmount = Ctx->getConstantInt(IceType_i32, 31);

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

nit: Since the later Shift is always needed, could have moved that earlier and
used that. Either way.

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

Done.

+        _mov(T_Lo, Src0RM);
+        _shl(T_Lo, ShiftAmount);
+        _sar(T_Lo, ShiftAmount);
+      } else {
         _movsx(T_Lo, Src0RM);
+      }
       _mov(DestLo, T_Lo);
       Variable *T_Hi = NULL;
       Constant *Shift = Ctx->getConstantInt(IceType_i32, 31);
@@ -1954,12 +1960,25 @@ void TargetX8632::lowerCast(const InstCast *Inst) {
         _movp(Dest, T);
       }
     } else {
-      // TODO: Sign-extend an i1 via "shl reg, 31; sar reg, 31", and
-      // also copy to the high operand of a 64-bit variable.
       // t1 = movsx src; dst = t1
-      Variable *T = makeReg(Dest->getType());
-      _movsx(T, Src0RM);
-      _mov(Dest, T);
+      // - or -
+      // t1 = src
+      // shl t1, dst_bitwidth - 1
+      // sar t1, dst_bitwidth - 1
+      // dst = t1
+      if (Src0RM->getType() == IceType_i1) {
+        size_t DestBits = CHAR_BIT * typeWidthInBytes(Dest->getType());

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

Should this be X86_CHAR_BIT instead (which the vector ShiftAmount uses above)?

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

Done.

+        Constant *ShiftAmount = Ctx->getConstantInt(IceType_i32, DestBits - 1);
+        Variable *T = NULL;
+        _mov(T, Src0RM);
+        _shl(T, ShiftAmount);
+        _sar(T, ShiftAmount);
+        _mov(Dest, T);
+      } else {
+        Variable *T = makeReg(Dest->getType());
+        _movsx(T, Src0RM);
+        _mov(Dest, T);
+      }
     }
     break;
   }
@@ -1971,15 +1990,20 @@ void TargetX8632::lowerCast(const InstCast *Inst) {
       Variable *DestLo = llvm::cast<Variable>(loOperand(Dest));
       Variable *DestHi = llvm::cast<Variable>(hiOperand(Dest));
       Variable *Tmp = makeReg(DestLo->getType());
-      if (Src0RM->getType() == IceType_i32)
+      if (Src0RM->getType() == IceType_i32) {
         _mov(Tmp, Src0RM);
-      else
+      } else if (Src0RM->getType() == IceType_i1) {
+        Constant *One = Ctx->getConstantInt(IceType_i32, 1);
+        _mov(Tmp, Src0RM);
+        _and(Tmp, One);
+      } else {
         _movzx(Tmp, Src0RM);
+      }
       _mov(DestLo, Tmp);
       _mov(DestHi, Zero);
     } else if (Src0RM->getType() == IceType_i1) {
       // t = Src0RM; t &= 1; Dest = t
-      Operand *One = Ctx->getConstantInt(IceType_i32, 1);
+      Constant *One = Ctx->getConstantInt(IceType_i32, 1);
       Variable *T = makeReg(IceType_i32);
       _movzx(T, Src0RM);
       _and(T, One);
@@ -1995,7 +2019,13 @@ void TargetX8632::lowerCast(const InstCast *Inst) {
     } else {
       // t1 = movzx src; dst = t1

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

Maybe move the t1 = movzx comment to the } else {

or add the "- or -" clause

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

Done (as part of the next comment below).

       Variable *T = makeReg(Dest->getType());
-      _movzx(T, Src0RM);
+      if (Src0RM->getType() == IceType_i1) {

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

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.

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

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.

+        Constant *One = Ctx->getConstantInt(IceType_i32, 1);
+        _mov(T, Src0RM);
+        _and(T, One);
+      } else {
+        _movzx(T, Src0RM);
+      }
       _mov(Dest, T);
     }
     break;
@@ -3920,7 +3950,7 @@ void TargetX8632::lowerSwitch(const InstSwitch *Inst) {
     Src0 = legalize(Src0, Legal_Reg | Legal_Mem, true);
   for (SizeT I = 0; I < NumCases; ++I) {
     // TODO(stichnot): Correct lowering for IceType_i64.
-    Operand *Value = Ctx->getConstantInt(IceType_i32, Inst->getValue(I));
+    Constant *Value = Ctx->getConstantInt(IceType_i32, Inst->getValue(I));

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

This seems separate. Add a comment in the commit message?

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

Done.

     _cmp(Src0, Value);
     _br(InstX8632Br::Br_e, Inst->getLabel(I));
   }