[PATCH] D38388: [DAGCombiner, x86] convert insertelement of bitcasted vector into shuffle

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:13787
+  // Step 3: Shuffle in the padded subvector.
+  return DAG.getVectorShuffle(VT, DL, DestVec, PaddedSubVec, ShufMaskVals);
+}
----------------
spatel wrote:
> spatel wrote:
> > RKSimon wrote:
> > > We need to test for isShuffleMaskLegal here and I think it will remove the need for shouldConvertInsSubVecToShuffle
> > Aha...I thought we must have something here already. I was trying something similar to that in an earlier draft, but it's tricky:
> > 
> > 1. If we use the actual shuffle type (VT), the subvector that we're trying to avoid messing with will already be cast from vXi1 to a potentially legal mask type. AVX512 disaster will occur for existing test cases.
> > 
> > 2. If we use the subvector type (SubVecVT), we will fail to get any of the cases shown here because the subvector type isn't legal (eg, v2i32). This is similar to why I couldn't use insert_subvector nodes - they require legal types.
> > 
> > 3. If we hack it to check for a hybrid fake type (the number of elements in the result + the element type of the subvector), we won't get the 512-bit cases for the AVX2 targets. This probably doesn't matter much in practice (shouldn't have larger than legal vectors in the first place?).
> > 
> > So it's not quite what we're looking for, but I can post a draft of option #3 if that's a reasonable alternative to a new hook. Or let me know if you see another way out.
> Note: option #3 isn't as crazy as I made it sound, that 'fake type' is ConcatVT in the current patch.
Don't the interim generated nodes need adding to the worklist?


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:13774
+  EVT ShufVT = EVT::getVectorVT(*DAG.getContext(), SubVecEltVT, NumMaskVals);
+  if (!DAG.getTargetLoweringInfo().isShuffleMaskLegal(Mask, ShufVT))
+    return SDValue();
----------------
How bad does it get if we allow any shuffle prior to legalization?


https://reviews.llvm.org/D38388