[llvm-commits] A patch fixing # 9173

[Duncan Sands]

Hi Nadav,

@@ -132,11 +132,27 @@
      if (const VectorType *SrcTy = dyn_cast<VectorType>(V->getType())) {
        assert(DestPTy->getBitWidth() == SrcTy->getBitWidth() &&
               "Not cast between same sized vectors!");
-      SrcTy = NULL;
        // First, check for null.  Undef is already handled.
        if (isa<ConstantAggregateZero>(V))
          return Constant::getNullValue(DestTy);

+      // In case of all-one vector, create a new all-one of dst type

If you are only going to do this when the source is an all-ones vector then
logic belongs in BitCastConstantVector.  You could also handle the case when
the source is an all-ones integer in which case at least that part belongs
here.

+      if (ConstantVector* CV = dyn_cast<ConstantVector>(V)) {
+        bool AllOne = true;
+        for (unsigned int i=0; i< SrcTy->getNumElements() ; ++i) {
+          ConstantInt *ci = dyn_cast<ConstantInt>(CV->getOperand(i));
+          if (! ci) {
+            AllOne = false;
+            break;
+          }
+          if (! ci->isAllOnesValue())  {
+            AllOne = false;
+            break;
+          }
+        }

This test for all-ones can be replaced with: CV->isAllOnesValue()

+        if (AllOne) Constant::getAllOnesValue(DestPTy);

Looks like you are missing the "return" keyword.


@@ -694,15 +710,37 @@
    if (isa<UndefValue>(Cond)) return V1;
    if (V1 == V2) return V1;

+  if (Cond->isNullValue()) return V2;

You could also check if CondV->isAllOnesValue() here and if so return V1.
You could also replace the ConstantInt Cond check at the start of this method
with something that checks isNullValue and isAllOnesValue.

+
+  if (ConstantVector* CondV = dyn_cast<ConstantVector>(Cond)) {
+    const VectorType *VTy = CondV->getType();
+    ConstantVector *CP1 = dyn_cast<ConstantVector>(V1);
+    ConstantVector *CP2 = dyn_cast<ConstantVector>(V2);


+
+    if ((CP1 != NULL || isa<ConstantAggregateZero>(V1)) &&
+      (CP2 != NULL || isa<ConstantAggregateZero>(V2))) {

Is it possible for this check to fail?  If not, it should be an assertion.

+
+      const Type *EltTy = V1->getType();
+      std::vector<Constant*> Res;

You know the size that Res will be so you can reserve the right amount of space
here.

+      for (unsigned i = 0, e = VTy->getNumElements(); i != e; ++i) {
+        ConstantInt* c = dyn_cast<ConstantInt>(CondV->getOperand(i));

What if CondV->getOperand(i) is a ConstantExpr and not a ConstantInt?  Then
you will get null here and crash below doing c->getZExtValue().

+        Constant *C1 = CP1 ? CP1->getOperand(i) : Constant::getNullValue(EltTy);
+        Constant *C2 = CP2 ? CP2->getOperand(i) : Constant::getNullValue(EltTy);
+        Res.push_back(c->getZExtValue() ? C1 : C2);
+      }
+     return ConstantVector::get(Res);
+    }
+  }
+
    if (ConstantExpr *TrueVal = dyn_cast<ConstantExpr>(V1)) {
      if (TrueVal->getOpcode() == Instruction::Select)
        if (TrueVal->getOperand(0) == Cond)
-	return ConstantExpr::getSelect(Cond, TrueVal->getOperand(1), V2);
+  return ConstantExpr::getSelect(Cond, TrueVal->getOperand(1), V2);

What's the reason for changing this line?

    }
    if (ConstantExpr *FalseVal = dyn_cast<ConstantExpr>(V2)) {
      if (FalseVal->getOpcode() == Instruction::Select)
        if (FalseVal->getOperand(0) == Cond)
-	return ConstantExpr::getSelect(Cond, V1, FalseVal->getOperand(2));
+  return ConstantExpr::getSelect(Cond, V1, FalseVal->getOperand(2));

Likewise.

Ciao, Duncan.