Improve bool folding

src/IceTargetLoweringX86BaseImpl.h

 //===- subzero/src/IceTargetLoweringX86BaseImpl.h - x86 lowering -*- C++ -*-==//
 //
 //                        The Subzero Code Generator
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 ///
 /// \file
@@ skipping 63 matching lines @@
 };
 
 template <class MachineTraits> class BoolFolding {
 public:
   enum BoolFoldingProducerKind {
     PK_None,
     // TODO(jpp): PK_Icmp32 is no longer meaningful. Rename to PK_IcmpNative.
     PK_Icmp32,
     PK_Icmp64,
     PK_Fcmp,
-    PK_Trunc
+    PK_Trunc,
+    PK_Arith  // A flag-setting arithmetic instruction.
   };
 
   /// Currently the actual enum values are not used (other than CK_None), but we
   /// go ahead and produce them anyway for symmetry with the
   /// BoolFoldingProducerKind.
   enum BoolFoldingConsumerKind { CK_None, CK_Br, CK_Select, CK_Sext, CK_Zext };
 
 private:
   BoolFolding(const BoolFolding &) = delete;
   BoolFolding &operator=(const BoolFolding &) = delete;
@@ skipping 23 matching lines @@
     : Instr(I), IsComplex(BoolFolding<MachineTraits>::hasComplexLowering(I)) {}
 
 template <class MachineTraits>
 typename BoolFolding<MachineTraits>::BoolFoldingProducerKind
 BoolFolding<MachineTraits>::getProducerKind(const Inst *Instr) {
   if (llvm::isa<InstIcmp>(Instr)) {
     if (MachineTraits::Is64Bit || Instr->getSrc(0)->getType() != IceType_i64)
       return PK_Icmp32;
     return PK_Icmp64;
   }
+  if (llvm::isa<InstFcmp>(Instr))
+    return PK_Fcmp;
+  if (auto *Arith = llvm::dyn_cast<InstArithmetic>(Instr)) {
+    if (MachineTraits::Is64Bit || Arith->getSrc(0)->getType() != IceType_i64) {
+      switch (Arith->getOp()) {
+      default:
+        break;

[Jim Stichnoth, 2015/11/11 14:05:20]

I think you should just "return PK_None", unless you expect to add the x86-32
i64 version in the future (in which case leave a TODO).

[sehr, 2015/11/13 06:00:52]

Done.

+      case InstArithmetic::And:
+      case InstArithmetic::Or:
+        return PK_Arith;
+      }
+    }
+  }
   return PK_None; // TODO(stichnot): remove this
 
-  if (llvm::isa<InstFcmp>(Instr))
-    return PK_Fcmp;
   if (auto *Cast = llvm::dyn_cast<InstCast>(Instr)) {
     switch (Cast->getCastKind()) {
     default:
       return PK_None;
     case InstCast::Trunc:
       return PK_Trunc;
     }
   }
   return PK_None;
 }
@@ skipping 1776 matching lines @@
   if (const class Inst *Producer = FoldingInfo.getProducerFor(Cond)) {
     assert(Producer->isDeleted());
     switch (BoolFolding::getProducerKind(Producer)) {
     default:
       break;
     case BoolFolding::PK_Icmp32:
     case BoolFolding::PK_Icmp64: {
       lowerIcmpAndBr(llvm::dyn_cast<InstIcmp>(Producer), Inst);
       return;
     }
+    case BoolFolding::PK_Fcmp: {
+      lowerFcmpAndBr(llvm::dyn_cast<InstFcmp>(Producer), Inst);
+      return;
+    }
+    case BoolFolding::PK_Arith: {
+      lowerArithAndBr(llvm::dyn_cast<InstArithmetic>(Producer), Inst);
+      return;
+    }
     }
   }
-
   Operand *Src0 = legalize(Cond, Legal_Reg | Legal_Mem);
   Constant *Zero = Ctx->getConstantZero(IceType_i32);
   _cmp(Src0, Zero);
   _br(Traits::Cond::Br_ne, Inst->getTargetTrue(), Inst->getTargetFalse());
 }
 
 template <class Machine>
 void TargetX86Base<Machine>::lowerCast(const InstCast *Inst) {
   // a = cast(b) ==> t=cast(b); a=t; (link t->b, link a->t, no overlap)
   InstCast::OpKind CastKind = Inst->getCastKind();
@@ skipping 592 matching lines @@
     ExtractedElementR = T;
   }
 
   // Copy the element to the destination.
   Variable *Dest = Inst->getDest();
   _mov(Dest, ExtractedElementR);
 }
 
 template <class Machine>
 void TargetX86Base<Machine>::lowerFcmp(const InstFcmp *Inst) {
+  constexpr InstBr *Br = nullptr;
+  lowerFcmpAndBr(Inst, Br);
+}
+
+template <class Machine>
+void TargetX86Base<Machine>::lowerFcmpAndBr(const InstFcmp *Inst,
+                                            const InstBr *Br) {
   Operand *Src0 = Inst->getSrc(0);
   Operand *Src1 = Inst->getSrc(1);
   Variable *Dest = Inst->getDest();
 
   if (isVectorType(Dest->getType())) {
+    if (Br)
+      llvm::report_fatal_error("vector compare/branch cannot be folded");
     InstFcmp::FCond Condition = Inst->getCondition();
     size_t Index = static_cast<size_t>(Condition);
     assert(Index < Traits::TableFcmpSize);
 
     if (Traits::TableFcmp[Index].SwapVectorOperands) {
       Operand *T = Src0;

[Jim Stichnoth, 2015/11/11 14:05:20]

std::swap(Src0, Src1);

[sehr, 2015/11/13 06:00:52]

Done.

       Src0 = Src1;
       Src1 = T;
     }
 
     Variable *T = nullptr;
 
     if (Condition == InstFcmp::True) {
       // makeVectorOfOnes() requires an integer vector type.
       T = makeVectorOfMinusOnes(IceType_v4i32);
     } else if (Condition == InstFcmp::False) {
@@ skipping 62 matching lines @@
   bool HasC1 = (Traits::TableFcmp[Index].C1 != Traits::Cond::Br_None);
   bool HasC2 = (Traits::TableFcmp[Index].C2 != Traits::Cond::Br_None);
   if (HasC1) {
     Src0 = legalize(Src0);
     Operand *Src1RM = legalize(Src1, Legal_Reg | Legal_Mem);
     Variable *T = nullptr;
     _mov(T, Src0);
     _ucomiss(T, Src1RM);
     if (!HasC2) {
       assert(Traits::TableFcmp[Index].Default);
-      _setcc(Dest, Traits::TableFcmp[Index].C1);
+      setccOrBr(Traits::TableFcmp[Index].C1, Dest, Br);
       return;
     }
   }
-  Constant *Default =
-      Ctx->getConstantInt(Dest->getType(), Traits::TableFcmp[Index].Default);
-  _mov(Dest, Default);
-  if (HasC1) {
-    typename Traits::Insts::Label *Label =
-        Traits::Insts::Label::create(Func, this);
-    _br(Traits::TableFcmp[Index].C1, Label);
-    if (HasC2) {
-      _br(Traits::TableFcmp[Index].C2, Label);
+  int32_t IntDefault = Traits::TableFcmp[Index].Default;
+  if (Br == nullptr) {
+    Constant *Default = Ctx->getConstantInt(Dest->getType(), IntDefault);
+    _mov(Dest, Default);
+    if (HasC1) {
+      typename Traits::Insts::Label *Label =
+          Traits::Insts::Label::create(Func, this);
+      _br(Traits::TableFcmp[Index].C1, Label);
+      if (HasC2) {
+        _br(Traits::TableFcmp[Index].C2, Label);
+      }
+      Constant *NonDefault = Ctx->getConstantInt(Dest->getType(), !IntDefault);
+      _mov_redefined(Dest, NonDefault);
+      Context.insert(Label);
     }
-    Constant *NonDefault =
-        Ctx->getConstantInt(Dest->getType(), !Traits::TableFcmp[Index].Default);
-    _mov_redefined(Dest, NonDefault);
-    Context.insert(Label);
+  } else {

[Jim Stichnoth, 2015/11/11 14:05:20]

Can this be "else if"?

  } else if (IntDefault == 0) {

[sehr, 2015/11/13 06:00:52]

I used std::swap and removed the duplication, as we discussed.

+    if (IntDefault == 0) {
+      if (HasC1) {
+        _br(Traits::TableFcmp[Index].C1, Br->getTargetFalse());
+        if (HasC2) {
+          _br(Traits::TableFcmp[Index].C2, Br->getTargetFalse());
+        }
+        _br(Br->getTargetTrue());
+        return;
+      }
+      _br(Br->getTargetFalse());
+    } else {
+      if (HasC1) {
+        _br(Traits::TableFcmp[Index].C1, Br->getTargetTrue());
+        if (HasC2) {
+          _br(Traits::TableFcmp[Index].C2, Br->getTargetTrue());
+        }
+        _br(Br->getTargetFalse());
+        return;
+      }
+      _br(Br->getTargetTrue());
+    }
   }
 }
 
 inline bool isZero(const Operand *Opnd) {
   if (auto *C64 = llvm::dyn_cast<ConstantInteger64>(Opnd))
     return C64->getValue() == 0;
   if (auto *C32 = llvm::dyn_cast<ConstantInteger32>(Opnd))
     return C32->getValue() == 0;
   return false;
 }
@@ skipping 289 matching lines @@
     // TODO(sehr,stichnot): This could be done with a single unconditional
     // branch instruction, but subzero doesn't know how to handle the resulting
     // control flow graph changes now.  Make it do so to eliminate mov and cmp.
     _mov(Dest, Ctx->getConstantInt(Dest->getType(), (IcmpResult ? 1 : 0)));
     _cmp(Dest, Ctx->getConstantInt(Dest->getType(), 0));
     _br(Traits::Cond::Br_ne, Br->getTargetTrue(), Br->getTargetFalse());
   }
 }
 
 template <class Machine>
+void TargetX86Base<Machine>::lowerArithAndBr(const InstArithmetic *Arith,
+                                             const InstBr *Br) {
+  Variable *T = nullptr;
+  Operand *Src0 = legalize(Arith->getSrc(0));
+  Operand *Src1 = legalize(Arith->getSrc(1));
+  Variable *Dest = Arith->getDest();
+  switch (Arith->getOp()) {
+  default:
+    llvm_unreachable("arithmetic operator not AND or OR");
+    break;
+  case InstArithmetic::And:
+    _mov(T, Src0);
+    _and(T, Src1);

[John, 2015/11/11 02:08:10]

would _test be better here? just curious.

[Jim Stichnoth, 2015/11/11 14:05:20]

Yes it would be better, since T would be able to share Src0's register.

[sehr, 2015/11/13 06:00:52]

Done.

[sehr, 2015/11/13 06:00:52]

Done.

+    break;
+  case InstArithmetic::Or:
+    _mov(T, Src0);
+    _or(T, Src1);
+    break;
+  }
+  Context.insert(InstFakeUse::create(Func, T));
+  Context.insert(InstFakeDef::create(Func, Dest));
+  _br(Traits::Cond::Br_ne, Br->getTargetTrue(), Br->getTargetFalse());
+}
+
+template <class Machine>
 void TargetX86Base<Machine>::lowerInsertElement(const InstInsertElement *Inst) {
   Operand *SourceVectNotLegalized = Inst->getSrc(0);
   Operand *ElementToInsertNotLegalized = Inst->getSrc(1);
   ConstantInteger32 *ElementIndex =
       llvm::dyn_cast<ConstantInteger32>(Inst->getSrc(2));
   // Only constant indices are allowed in PNaCl IR.
   assert(ElementIndex);
   unsigned Index = ElementIndex->getValue();
   assert(Index < typeNumElements(SourceVectNotLegalized->getType()));
 
@@ skipping 2908 matching lines @@
   }
   // the offset is not eligible for blinding or pooling, return the original
   // mem operand
   return MemOperand;
 }
 
 } // end of namespace X86Internal
 } // end of namespace Ice
 
 #endif // SUBZERO_SRC_ICETARGETLOWERINGX86BASEIMPL_H