[PATCH] fix for bug 18319

[Nick Lewycky]

On 12/27/2013 06:45 AM, Ilia Filippov wrote:
> Here is an updated patch, please review

The new logic looks great, thanks!

These two lines are over 80 columns:

+      Constant *V1Element = ConstantExpr::getExtractElement(V1, 
ConstantInt::get(Ty, i));
+      Constant *V2Element = ConstantExpr::getExtractElement(V2, 
ConstantInt::get(Ty, i));

+      if (V1Element == V2Element)
+        V = V1Element;
+      else if (isa<UndefValue>(Cond))
+        V = isa<UndefValue>(V1Element) ? V1Element : V2Element;
+      else {

Please add braces around each stanza, even though they're only 1 line. 
The idea is that one of the sides (the final else in this case) has 
braces, so too should the other stanzas.

And you need to include a testcase.

Nick

> 2013/12/25 Nick Lewycky <nicholas at mxc.ca <mailto:nicholas at mxc.ca>>
>
>     On 12/24/2013 07:18 AM, Ilia Filippov wrote:
>
>         Hello,
>
>         Could somebody please review and commit my fix to bug
>         http://llvm.org/bugs/show_bug.__cgi?id=18319
>         <http://llvm.org/bugs/show_bug.cgi?id=18319> ?
>         (patch is attached).
>
>         The fix handles the case of vector select constant expression
>         containing
>         "undef" values in ConstantFoldSelectInstruction function.
>
>
>     Index: lib/IR/ConstantFold.cpp
>     ==============================__==============================__=======
>     --- lib/IR/ConstantFold.cpp     (revision 197903)
>     +++ lib/IR/ConstantFold.cpp     (working copy)
>     @@ -706,9 +706,15 @@
>           Type *Ty = IntegerType::get(CondV->__getContext(), 32);
>           for (unsigned i = 0, e =
>     V1->getType()->__getVectorNumElements(); i != e;++i){
>             ConstantInt *Cond =
>     dyn_cast<ConstantInt>(CondV->__getOperand(i));
>     -      if (Cond == 0) break;
>     -
>     -      Constant *V = Cond->isNullValue() ? V2 : V1;
>     +      Constant *V;
>     +      if (Cond == 0) {
>     +        if (isa<UndefValue>(CondV->__getOperand(i)))
>
>     Instead of re-getting from CondV, make Cond a Constant* instead of a
>     ConstantInt*, and have it do "if (!isa<ConstantInt>(Cond)) break;"
>     after handling the undef case.
>
>     There's some other logic missing here though. If
>     cast<ConstantVector>(V1)->__getOperand(n) is equal to
>     cast<ConstantVector>(V2)->__getOperand(n), then we don't care what
>     Cond is at all, we should just pick that element.
>
>     Since we're going to look at each element this way, why not check if
>     one of the elements is better than the other (ie., is an undef) and
>     then pick that element when the condition is undef?
>
>     Nick
>
>     +          V = V2;
>     +        else
>     +          break;
>     +      }
>     +      else
>     +        V = Cond->isNullValue() ? V2 : V1;
>             Constant *Res = ConstantExpr::__getExtractElement(V,
>     ConstantInt::get(Ty, i));
>             Result.push_back(Res);
>           }
>
>     _________________________________________________
>     llvm-commits mailing list
>     llvm-commits at cs.uiuc.edu <mailto:llvm-commits at cs.uiuc.edu>
>     http://lists.cs.uiuc.edu/__mailman/listinfo/llvm-commits
>     <http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits>
>
>