[PATCH] D27787: Restrict some DAGCombines for SHUFFLE_VECTOR nodes.

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:14099
   SDValue N1 = SVN->getOperand(1);
 
+  if (!isAnyConstantBuildVector(N0.getNode()))
----------------
mkuper wrote:
> Just to make sure I understand - the reason you're removing the hasOneUse constraint is that for the case where the BUILD_VECTOR ends up a constant pool load, it's better to just load the combined vector instead of shuffling even if other users exist?
We might end up with something slightly more expensive in some cases, but it seems like a worthwhile tradeoff in most situations.  The hasOneUse() check won't reliably preserve constants anyway; other parts of the code like to fold away bitcasts and insert undefs into constants.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:14361
     ShuffleVectorSDNode *OtherSV = cast<ShuffleVectorSDNode>(N0);
+    if (OtherSV->isSplat())
+      return SDValue();
----------------
mkuper wrote:
> Is there a reason this has to land in the same patch as the other change?
> Also, could you explain why you consider splats free? On x86, at least, I don't see why that would be the case.
I can land it separately, sure.

Not all broadcasts are free, but some are; for example, on x86 with AVX2, a splat can be folded into a load to form vpbroadcastb.  I'll add an x86 testcase which covers this.


================
Comment at: test/CodeGen/ARM/vzip.ll:270
 entry:
   ; CHECK-LABEL: vzip_lower_shufflemask_undef
   ; CHECK: vzip
----------------
mkuper wrote:
> Regenerate this with the update script?
There is no update script for ARM, as far as I know; it's possible I should spend some time writing one.


================
Comment at: test/CodeGen/X86/mmx-bitcast.ll:83
 ; CHECK-NEXT:    punpckldq {{.*#+}} xmm1 = xmm1[0],xmm0[0],xmm1[1],xmm0[1]
-; CHECK-NEXT:    movd %xmm1, %rax
+; CHECK-NEXT:    pshufd {{.*#+}} xmm0 = xmm1[0,1,1,3]
+; CHECK-NEXT:    movd %xmm0, %rax
----------------
mkuper wrote:
> This looks like a common enough pattern that I'm not excited about regressing it at the expense of the "zip" example.
Mmm... maybe.  I would appreciate suggestions.


Repository:
  rL LLVM

https://reviews.llvm.org/D27787