[PATCH] D30176: [X86][SSE] Attempt to extract vector elements through target shuffles

[Filipe Cabecinhas via Phabricator via llvm-commits]

filcab added inline comments.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:28877
+  // Don't attempt this for mask vectors or unknown extraction indices.
+  if ((SrcSVT.getSizeInBits() % 8) != 0 || !isa<ConstantSDNode>(Idx))
+    return SDValue();
----------------
What if you have a mask vector of 8 bits? Maybe check for `SrcSVT == i1`?


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:28922
+      ((SrcIdx == 0 && Subtarget.hasSSE2()) || Subtarget.hasSSE41())) {
+    assert(SrcSVT == VT && "Unexpected extraction type");
+    return DAG.getNode(ISD::EXTRACT_VECTOR_ELT, dl, SrcSVT, SrcOp,
----------------
Seems to me this would still be valid with different types (extract_element with a different type), but we can't do anything in that case. If so, shouldn't we `return SDValue();` instead of asserting?


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:34441
+  case X86ISD::PEXTRB:
+    return combineExtractVectorElt_SSE(N, DAG, DCI, Subtarget);
   case ISD::INSERT_SUBVECTOR:
----------------
Is this an optimization or a correctness thing? Why can't you simply call `combineExtractVectorElt` since it will try that combine anyway?


================
Comment at: test/CodeGen/X86/promote-vec3.ll:19
+; SSE3-NEXT:    pextrw $1, %xmm0, %edx
+; SSE3-NEXT:    pextrw $2, %xmm0, %ecx
 ; SSE3-NEXT:    punpcklwd {{.*#+}} xmm0 = xmm0[0],xmm1[0],xmm0[1],xmm1[1],xmm0[2],xmm1[2],xmm0[3],xmm1[3]
----------------
Interesting.


Repository:
  rL LLVM

https://reviews.llvm.org/D30176