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

src/IceTargetLoweringX8632.cpp

 //===- subzero/src/IceTargetLoweringX8632.cpp - x86-32 lowering -----------===//
 //
 //                        The Subzero Code Generator
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This file implements the TargetLoweringX8632 class, which
@@ skipping 1903 matching lines @@
     // allow an immediate operand.  If the operand is an immediate, we could
     // consider computing the strength-reduced result at translation time,
     // but we're unlikely to see something like that in the bitcode that
     // the optimizer wouldn't have already taken care of.
     Operand *Src0RM = legalize(Inst->getSrc(0), Legal_Reg | Legal_Mem);
     if (Dest->getType() == IceType_i64) {
       // t1=movsx src; t2=t1; t2=sar t2, 31; dst.lo=t1; dst.hi=t2
       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);
       _mov(T_Hi, T_Lo);
       _sar(T_Hi, Shift);
       _mov(DestHi, T_Hi);
     } else if (isVectorType(Dest->getType())) {
       Type DestTy = Dest->getType();
       if (DestTy == IceType_v16i8) {
         // onemask = materialize(1,1,...); dst = (src & onemask) > 0
         Variable *OneMask = makeVectorOfOnes(Dest->getType());
         Variable *T = makeReg(DestTy);
         _movp(T, Src0RM);
         _pand(T, OneMask);
         Variable *Zeros = makeVectorOfZeros(Dest->getType());
         _pcmpgt(T, Zeros);
         _movp(Dest, T);
       } else {
         // width = width(elty) - 1; dest = (src << width) >> width
         SizeT ShiftAmount =
             X86_CHAR_BIT * typeWidthInBytes(typeElementType(DestTy)) - 1;
         Constant *ShiftConstant = Ctx->getConstantInt(IceType_i8, ShiftAmount);
         Variable *T = makeReg(DestTy);
         _movp(T, Src0RM);
         _psll(T, ShiftConstant);
         _psra(T, ShiftConstant);
         _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;
   }
   case InstCast::Zext: {
     Operand *Src0RM = legalize(Inst->getSrc(0), Legal_Reg | Legal_Mem);
     if (Dest->getType() == IceType_i64) {
       // t1=movzx src; dst.lo=t1; dst.hi=0
       Constant *Zero = Ctx->getConstantZero(IceType_i32);
       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);
       _mov(Dest, T);
     } else if (isVectorType(Dest->getType())) {
       // onemask = materialize(1,1,...); dest = onemask & src
       Type DestTy = Dest->getType();
       Variable *OneMask = makeVectorOfOnes(DestTy);
       Variable *T = makeReg(DestTy);
       _movp(T, Src0RM);
       _pand(T, OneMask);
       _movp(Dest, T);
     } 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;
   }
   case InstCast::Trunc: {
     if (isVectorType(Dest->getType())) {
       // onemask = materialize(1,1,...); dst = src & onemask
       Operand *Src0RM = legalize(Inst->getSrc(0), Legal_Reg | Legal_Mem);
       Type Src0Ty = Src0RM->getType();
       Variable *OneMask = makeVectorOfOnes(Src0Ty);
@@ skipping 1904 matching lines @@
   Operand *Src0 = Inst->getComparison();
   SizeT NumCases = Inst->getNumCases();
   // OK, we'll be slightly less naive by forcing Src into a physical
   // register if there are 2 or more uses.
   if (NumCases >= 2)
     Src0 = legalizeToVar(Src0, true);
   else
     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));
   }
 
   _br(Inst->getLabelDefault());
 }
 
 void TargetX8632::scalarizeArithmetic(InstArithmetic::OpKind Kind,
                                       Variable *Dest, Operand *Src0,
                                       Operand *Src1) {
@@ skipping 485 matching lines @@
     Str << "\t.align\t" << Align << "\n";
     Str << MangledName << ":\n";
     for (SizeT i = 0; i < Size; ++i) {
       Str << "\t.byte\t" << (((unsigned)Data[i]) & 0xff) << "\n";
     }
     Str << "\t.size\t" << MangledName << ", " << Size << "\n";
   }
 }
 
 } // end of namespace Ice