[llvm] r182976 - Reapply with r182909 with a fix to the calculation of the new indices for

[Nadav Rotem]

Hi Nick, 

Thanks for working on this. I have a few comments. 

> 
> URL: http://llvm.org/viewvc/llvm-project?rev=182976&view=rev
> Log:
> Reapply with r182909 with a fix to the calculation of the new indices for
> insertelement instructions.
> 

Can you use the commit message to describe what the patch does ? 


> }
> 
> +/// Return true if we can evaluate the specified expression tree if the vector
> +/// elements were shuffled in a different order.
> +static bool CanEvaluateShuffled(Value *V, ArrayRef<int> Mask,
> +                                unsigned Depth = 100) {

Depth = 100 ?? This is really high. It should be a much lower threshold.  This can potentially search 2^100 paths because you are calling this recursion twice on binary operands.  Please limit this to something like 5. 

> 
> +    case Instruction::InsertElement: {
> +      ConstantInt *CI = dyn_cast<ConstantInt>(I->getOperand(2));
> +      if (!CI) return false;
> +      int ElementNumber = CI->getLimitedValue();
> +
> +      // Verify that 'CI' does not occur twice in Mask. A single 'insertelement'
> +      // can't put an element into multiple indices.
> +      bool SeenOnce = false;
> +      for (int i = 0, e = Mask.size(); i != e; ++i) {
> +        if (Mask[i] == ElementNumber) {
> +          if (SeenOnce)
> +            return false;
> +          SeenOnce = true;
> +        }
> +      }
> +      return CanEvaluateShuffled(I->getOperand(0), Mask, Depth-1);
> +    }
> +  }
> +  return false;

I would like to see more tests for the code that deals with insertelements. What happens if only some of the lanes are inserted ?  Does it ever fail ?

>   }
> 
> +  if (isa<UndefValue>(RHS) &&
> +      // This isn't necessary for correctness, but the comment block below
> +      // claims that there are cases where folding two shuffles into one would
> +      // cause worse codegen on some targets.
> +      !isa<ShuffleVectorInst>(LHS) &&
> +      CanEvaluateShuffled(LHS, Mask)) {

What ?

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130530/070ae9be/attachment.html>