[PATCH] D33137: [DAGCombiner] use narrow vector ops to eliminate concat/extract (PR32790)

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:14470
+         "Extract index is not a multiple of the vector length.");
+  if (Extract->getOperand(0).getValueSizeInBits() != VT.getSizeInBits() * 2)
+    return SDValue();
----------------
RKSimon wrote:
> Could you use isExtractSubvectorCheap instead here?
> 
> We might even need a isExtractSubvectorFree option as well...
I might have missed the intent of the question, but I don't think we want to limit the transform based on whether the extract is cheap or not. If this succeeds, then we're going to bypass the extract completely. The reason for limiting to 1/2 size extract is because, for example, a 1/4 extract sequence might become:
  W = wideop (concat X1, X2, X3, X4), Y
  N1 = extract W, 1 
  N2 = extract W, 2 
  N3 = extract W, 3 
  N4 = extract W, 4

  N1 = binop X1, (extract Y, 1)
  N2 = binop X2, (extract Y, 2)
  N3 = binop X3, (extract Y, 3)
  N4 = binop X4, (extract Y, 4)

In that case, we may have increased the instruction count, so we'd need some kind of cost calc to know if it's worthwhile. It would be a good transform for the case where we know that both X and Y are formed from concats, but I figured we should leave that for later...because it makes the code even more complicated!


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:14476
+  SDValue BinOp = Extract->getOperand(0);
+  if (BinOp.getOpcode() == ISD::BITCAST)
+    BinOp = BinOp.getOperand(0);
----------------
RKSimon wrote:
> I'm starting to think that X86ISelLowering's peekThroughBitcasts and peekThroughOneUseBitcasts helpers should be exposed globally.
Probably a good idea. Although we always fold a bitcast-of-a-bitcast, right? I've never seen a need for those to loop.


https://reviews.llvm.org/D33137