[llvm-commits] A patch fixing # 9173

[Rotem, Nadav]

Hi Duncan, 

Thank you for the quick review. I made the corrections and added two regression tests. 
Patch attached. 

Thanks, 
Nadav

-----Original Message-----
From: llvm-commits-bounces at cs.uiuc.edu [mailto:llvm-commits-bounces at cs.uiuc.edu] On Behalf Of Duncan Sands
Sent: Wednesday, February 09, 2011 18:18
To: llvm-commits at cs.uiuc.edu
Subject: Re: [llvm-commits] A patch fixing # 9173

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.
_______________________________________________
llvm-commits mailing list
llvm-commits at cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: constant_fold_cast.diff
Type: application/octet-stream
Size: 4392 bytes
Desc: constant_fold_cast.diff
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20110209/cab4a367/attachment.obj>