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

[Gabor Buella via Phabricator via llvm-commits]

GBuella added inline comments.


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


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:9483
+  if (OriginalVector.getSimpleValueType().getVectorElementType() == MVT::i16 &&
+      VT == MVT::v16i8 && !Subtarget.hasBWI())
+    return SDValue();
----------------
RKSimon wrote:
> Why is it just 16i8 and not 32i8 as well for _mm512_cvtepi16_epi8 ?
This part is only about truncations, where the result must be filled with extra zeros, due to the (narrower tan 128bits) result being in an xmm register.
The  _mm512_cvtepi16_epi8 one truncates from a 512bit vector into a 256bit vector, that is already recognized without this patch.

The check here is about _mm_cvtepi16_epi8  (which requires avx512vl & avx512bw). It truncates from v8i16 -> v8i8, but the vpmovwb instruction actually sets a whole xmm register, so the actual result is going to be v16i8, with other 8 bytes set to zero.

Ok, perhaps these details should be explained in comments around here.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:9492
+  if (maskContainsSequenceForVPMOV(Mask, SwappedOps, 2) &&
+      maskContainsSequenceForVPMOV(Mask, SwappedOps, 4))
+    return SDValue();
----------------
RKSimon wrote:
> Is this right? I'd expect it to check for the ! case.
Ye, originally it was:
```
  if (!maskContainsSequenceForVPMOV(Mask, SwappedOps, 2) &&
      !maskContainsSequenceForVPMOV(Mask, SwappedOps, 4))
    return SDValue();
```
The new form of this conjuction doesn't seem to make much sense at first sight, what happened?


https://reviews.llvm.org/D46957