[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;
+  }


On Feb 17, 2014, at 7:26 AM, Tim Northover <t.p.northover at gmail.com> wrote:

> <D2816.1.patch>