[PATCH] D104156: [DAGCombine][X86][ARM] EXTRACT_SUBVECTOR(VECTOR_SHUFFLE(?,?,Mask)) -> VECTOR_SHUFFLE(EXTRACT_SUBVECTOR(?, ?), EXTRACT_SUBVECTOR(?, ?), Mask')

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added a comment.

Do we have any test coverage with any subvectors that are not half the width of the source? 512-bit -> 128-bit AVX512 shuffles for instance.



================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:20620
+
+    assert(M < (2 * WideNumElts) && "Out-of-bounds shuffle mask?");
+
----------------
Merge the 2 asserts?

assert((-1 <= M) && (M < (2 * WideNumElts)) && "Out-of-bounds shuffle mask?");


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:20651
+    // Profitability check: only deal with extractions from the first subvector.
+    if (OpSubvecIdx != 0)
+      return SDValue();
----------------
This is actually pretty AVX specific - for instance NEON can usually reference both 64-bit vectors for free.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:20654
+
+    const std::pair<SDValue, int> DemandedSubvector(Op, OpSubvecIdx);
+
----------------
make_pair?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:20670
+  assert(DemandedSubvectors.size() <= 2 &&
+         "Should have ended up demanding at most two subvectors.");
+
----------------
There are a lot of assertions in this function :) 


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:20684
+      !TLI.isOperationLegalOrCustom(ISD::VECTOR_SHUFFLE, NarrowVT))
+    return SDValue();
+
----------------
Shouldn't this be done earlier as soon as we know NarrowVT to early-out ?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104156/new/

https://reviews.llvm.org/D104156