[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:28922
+      ((SrcIdx == 0 && Subtarget.hasSSE2()) || Subtarget.hasSSE41())) {
+    assert(SrcSVT == VT && "Unexpected extraction type");
+    return DAG.getNode(ISD::EXTRACT_VECTOR_ELT, dl, SrcSVT, SrcOp,
----------------
RKSimon wrote:
> filcab wrote:
> > 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?
> It shouldn't happen as we test for after legalization at the start of the function, but I can replace it with an early out.
If it's a "shouldn't ever happen", as it seems to be, then the assert is best.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:34441
+  case X86ISD::PEXTRB:
+    return combineExtractVectorElt_SSE(N, DAG, DCI, Subtarget);
   case ISD::INSERT_SUBVECTOR:
----------------
RKSimon wrote:
> filcab wrote:
> > Is this an optimization or a correctness thing? Why can't you simply call `combineExtractVectorElt` since it will try that combine anyway?
> combineExtractVectorElt has a load of other combines inside it (notably load extraction) that can't handle PEXTRW/PEXTRB and their implicit zeroing of the upper bits of their i32 result. I'll make this clearer in the TODO at the top of combineExtractVectorElt_SSE
Thanks.


Repository:
  rL LLVM

https://reviews.llvm.org/D30176