[PATCH] D11218: AVX512 : Integer Truncate with/without saturation support

[Igor Breger]

igorb added inline comments.

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:15239
@@ +15238,3 @@
+                                    SelectionDAG &DAG,
+                                    bool extendMask = false) {
+    EVT VT     = Op.getValueType();
----------------
ab wrote:
> capitalization? extend -> Extend (here and elsewhere)
> 
> But I'm not convinced it's a good idea to reuse the same function.  Perhaps it's different enough to warrant another function for masking truncstores?
extendMask flag removed

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:15278
@@ +15277,3 @@
+      case X86ISD::VTRUNCUS:
+        OpcodeSelect = X86ISD::SELECT;
+        break;
----------------
ab wrote:
> Why? I see the patterns match X86select, but what's wrong with ISD::VSELECT?
ISD::VSELECT can be lower and combined later to different sequence ( broadcast for example), to prevent this behavior i use X86ISD::SELECT

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:15416
@@ +15415,3 @@
+      return getVectorMaskingNode(DAG.getNode(IntrData->Opc0, dl, VT, Src),
+                                  Mask, Passthru, Subtarget, DAG, true);
+    }
----------------
ab wrote:
> true -> /*ExtendMask=*/true
case removed

================
Comment at: lib/Target/X86/X86InstrAVX512.td:5786-5788
@@ +5785,5 @@
+
+defm VPMOVQB    : avx512_trunc_qb<0x32, "vpmovqb", X86vtrunc>;
+defm VPMOVSQB   : avx512_trunc_sat_qb<0x22, "s",   X86vtruncs>;
+defm VPMOVUSQB  : avx512_trunc_sat_qb<0x12, "us",  X86vtruncus>;
+
----------------
ab wrote:
> Multiclass?  !add(0x10) or something for the opcode.
I think that in this case a multiclass would not help here, it will not decrease the code lines numbers and most probably make code more difficult for understanding.

================
Comment at: lib/Target/X86/X86IntrinsicsInfo.h:178
@@ -141,1 +177,3 @@
+  X86_INTRINSIC_DATA(avx512_mask_pmov_wb_mem_512, TRUNCATE_TO_MEM_VI8,
+                     X86ISD::VTRUNC, 0),
   X86_INTRINSIC_DATA(avx512_scatter_dpd_512, SCATTER, X86::VSCATTERDPDZmr, 0),
----------------
ab wrote:
> What about the saturating _mem variants? If you add them, would that let you remove the avx512_trunc_mr_lowering patterns?
Yes , you are correct, it will also simplify the implementation . But in order to do this i need to extend  StoreSDNode and MaskedStoreSDNode class to hold additional  information  - saturation type. 
Do you think it is a good idea to extend these classes?


Repository:
  rL LLVM

http://reviews.llvm.org/D11218