[PATCH] D134477: [X86] Lower vector interleave into unpck and perm

[Han Zhu via Phabricator via llvm-commits]

zhuhan0 added inline comments.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:17841
+    DAG.ReplaceAllUsesWith(SecondHalf, &Perm2);
+    return Perm1;
+  }
----------------
RKSimon wrote:
> Why are the ReplaceAllUsesWith calls necessary? We usually just rely on combines / demandedelts to replace these.
Because in [SelectionDAGLegalize::LegalizeOp](https://github.com/llvm/llvm-project/blob/main/llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp#L1304) only the currently visited node is replaced in the DAG, but here we're trying to transform both vector shuffle nodes, so I replaced the other (not currently visited) node on the fly. This does seem weird so I'm happy to change it if there's a better way. Is there an example of using "combines / demandedelts" to replace multiple nodes at the same time? 


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:18513
+          lowerShufflePairAsUNPCKAndPermute(DL, MVT::v32i8, V1, V2, Mask, DAG))
+    return V;
+
----------------
RKSimon wrote:
> I'm still not certain if we're better off trying to perform this in lowering or via shuffle combining or maybe combineConcatVectorOps?
We had the same question. The thing is, the original v64i8 vector_shuffle is split during generic dag combine, so it never gets to the X86 dag combiner.

After the v64i8 shuffle is split, the v32i8 shuffles do get to the X86 dag combiner, and they go through the `combineShuffle` path. That does come earlier than the legalizer. But `combineShuffle` doesn't call `combineConcatVectorOps` though and I'm not sure if any existing combine there is a good place to add this change. I can try adding a new combine there if you think that's a better place.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134477