[PATCH] D46957: [x86] Lower some trunc + shuffle patterns to vpmov[q|d][b|w]

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added inline comments.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:9406
 
+static bool maskContainsSequenceForVPMOV(ArrayRef<int> Mask, bool SwappedOps,
+                                         int delta) {
----------------
We've usually called these functions something like 'matchVectorShuffleAsVPMOV' or 'matchVectorShuffleAsVTRUNC'


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:9407
+static bool maskContainsSequenceForVPMOV(ArrayRef<int> Mask, bool SwappedOps,
+                                         int delta) {
+  int Size = (int)Mask.size();
----------------
int Delta


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:9417
+  // The rest of the mask should not refer to the truncated vector's elements.
+  if (isAnyInRange(Mask.slice(Split, Size - Split), TruncatedVectorStart,
+                   TruncatedVectorStart + Size))
----------------
I still don't get why you don't want to just use isUndefOrInRange with the 'safe' vector range.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:9473
+
+  SDValue OriginalVector = V1.getOperand(0).getOperand(0);
+
----------------
Save yourself some typing ;-)
```
SDValue Src = V1.getOperand(0).getOperand(0);
MVT SrcVT = Src.getSimpleValueType();
```


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:9483
+  if (OriginalVector.getSimpleValueType().getVectorElementType() == MVT::i16 &&
+      VT == MVT::v16i8 && !Subtarget.hasBWI())
+    return SDValue();
----------------
Why is it just 16i8 and not 32i8 as well for _mm512_cvtepi16_epi8 ?


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:9487
+  if (Mask.size() != VT.getVectorNumElements())
+    return SDValue();
+
----------------
Couldn't this be at the top for an early-out?


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:9492
+  if (maskContainsSequenceForVPMOV(Mask, SwappedOps, 2) &&
+      maskContainsSequenceForVPMOV(Mask, SwappedOps, 4))
+    return SDValue();
----------------
Is this right? I'd expect it to check for the ! case.


https://reviews.llvm.org/D46957