[PATCH] D72361: [DAGCombiner] reduce extract subvector of concat

[Sanjay Patel via Phabricator via llvm-commits]

spatel marked an inline comment as done.
spatel added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:18591
+    //   v2i8 extract_subvec (v16i8 concat (v8i8 X), (v8i8 Y), 14 -->
+    //   v2i8 extract_subvec v8i8 Y, 6
+    if (ConcatSrcNumElts > ExtNumElts) {
----------------
spatel wrote:
> lebedev.ri wrote:
> > I'm having a hard time parsing this.
> > 
> > Did we ensure that the original extract only extracts
> > only from one single concatenated vector?
> > 
> > I.e. that it does not take a new elements from
> > the end of first concatenated vector, and a few elements
> > from the beginning of second concatenated vector?
> > (That case could be implemented as a shuffle if we concatenating the same vector)
> > 
> I think this goes back to the assert mentioned above for extract_subvector. It's not legal to have something like this:
> v2i8 extract_subvec (v16i8 X), 7
> 
> Because the index operand must be a multiple of the final vector length (7 % 2 != 0).
> So that means it is impossible to extract from more than 1 operand of the concat (because the concat operands are guaranteed to be longer than this extract result).
> 
> I can add the above as a code comment if it helps. We could also assert that ExtNumElts % NewExtIdx == 0?
Aha - I wasn't being imaginative enough. My assumption was that the concat vector ops are some multiple of the final vector size rather than just larger.

So yes, it is possible to straddle concat ops if we have something like this:
v4i8 extract_subvec (v12i8 concat (v6i8 X), (v6i8 Y), 4

I'll fix the predicate and add an assert.


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

https://reviews.llvm.org/D72361