[PATCH] X86: sink splat-shuffle into block doing a shift.

[Nadav Rotem]

  Hi Tim!

  Thanks for working on this. I think that your approach makes sense. Did you look at the AVX512 instruction set?

   A few comments below:

  +bool X86TargetLowering::isVariableVectorShiftExpensive(Type *Ty) const {
  +  // Before SSE2 there are no vector shift instructions that LLVM can use, so it
  +  // doesn't particularly matter either way whether a shift is variable or not.
  +  if (!Subtarget->hasSSE2())
  +    return false;
  +
  +  // With SSE2 and above, there are no 8-bit shifts by vector or scalar: don't
  +  // take special steps.
  +  unsigned Bits = Ty->getScalarSizeInBits();
  +  if (Bits == 8)
  +    return false;
  +
  +  // SSE2 introduced vector-shift-by-scalar instructions (e.g psllw), but before
  +  // AVX2 there were no variable shift instructions so it's worth sinking splat
  +  // shuffles so they're visible to CodeGen.
  +  if (!Subtarget->hasInt256())
  +    return true;
  +
  +  // With AVX2 there are 32-bit and 64-bit variable shifts so they're not
  +  // expensive. 8-bit shifts were handled above, leaving 16-bit shifts. It turns
  +  // out scalar is still preferable there.
  +  return Bits == 16;
  +}
  +


  I think we can handle the 8bit and 16bit together.  I would just write this long function like this:

  bool X86TargetLowering::isVariableVectorShiftExpensive(Type *Ty) const {
   if (Subtarget->hasInt256() && (Bits == 32 || Bits == 64))
     return false;

    return true;
  }


  This can be extracted into a helper function - isBroadcast / isSplat.

  +  SmallVector<int, 16> Mask(SVI->getShuffleMask());
  +  int SplatElem = -1;
  +  for (unsigned i = 0; i < Mask.size(); ++i) {
  +    if (SplatElem != -1 && Mask[i] != -1 && Mask[i] != SplatElem)
  +      return false;
  +    SplatElem = Mask[i];
  +  }


  Do we really need this check?  Won’t SelectionDAG CSE it for us?

  +  // InsertedShuffles - Only insert a shuffle in each block once.
  +  DenseMap<BasicBlock*, Instruction*> InsertedShuffles;
  +
  +  bool MadeChange = false;
  +  for (Value::use_iterator UI = SVI->use_begin(), E = SVI->use_end();
  +       UI != E; ++UI) {
  +    Instruction *User = cast<Instruction>(*UI);

  Won’t SDAG do it for us?

  +  // If we removed all uses, nuke the shuffle.
  +  if (SVI->use_empty()) {
  +    SVI->eraseFromParent();
  +    MadeChange = true;
  +  }

http://llvm-reviews.chandlerc.com/D2816