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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:20651
+    // Profitability check: only deal with extractions from the first subvector.
+    if (OpSubvecIdx != 0)
+      return SDValue();
----------------
RKSimon wrote:
> This is actually pretty AVX specific - for instance NEON can usually reference both 64-bit vectors for free.
Right, this needs relaxation. I intend to at least special-handle the case
where we are extracting from vector concatenation,
then in certain cases subvec idx is also irrelevant.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:20670
+  assert(DemandedSubvectors.size() <= 2 &&
+         "Should have ended up demanding at most two subvectors.");
+
----------------
RKSimon wrote:
> There are a lot of assertions in this function :) 
Generally i feel like there are a lot of invariants in LLVM codebase that aren't asserted.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:20684
+      !TLI.isOperationLegalOrCustom(ISD::VECTOR_SHUFFLE, NarrowVT))
+    return SDValue();
+
----------------
RKSimon wrote:
> Shouldn't this be done earlier as soon as we know NarrowVT to early-out ?
Well, yes and no. As it can be seen in the comment for the previous code block,
we then would loose `return DAG.getUNDEF(NarrowVT);` constant-fold case.
Should we not have it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104156