[PATCH] D32273: [X86][AVX512] Make i1 illegal in the CodeGen

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: lib/Target/X86/X86CallingConv.td:76
 
     // Promote i1/i8/i16 arguments to i32.
+    CCIfType<[v1i1, i8, i16], CCPromoteToType<i32>>,
----------------
Update comment


================
Comment at: lib/Target/X86/X86CallingConv.td:77
     // Promote i1/i8/i16 arguments to i32.
+    CCIfType<[v1i1, i8, i16], CCPromoteToType<i32>>,
     CCIfType<[i1, i8, i16], CCPromoteToType<i32>>,
----------------
Merge with the line below and have 4 types?


================
Comment at: lib/Target/X86/X86CallingConv.td:150
 def RetCC_#NAME : CallingConv<[
     // Promote i1, v8i1 arguments to i8.
+    CCIfType<[v1i1, v8i1], CCPromoteToType<i8>>,
----------------
Update comment


================
Comment at: lib/Target/X86/X86CallingConv.td:151
     // Promote i1, v8i1 arguments to i8.
+    CCIfType<[v1i1, v8i1], CCPromoteToType<i8>>,
     CCIfType<[i1, v8i1], CCPromoteToType<i8>>,
----------------
Shouldn't this be merge with the line below to list 3 types?


================
Comment at: lib/Target/X86/X86CallingConv.td:492
 
   // Promote i1/i8/i16 arguments to i32.
+  CCIfType<[v1i1, i8, i16], CCPromoteToType<i32>>,
----------------
Update comment.


================
Comment at: lib/Target/X86/X86CallingConv.td:494
+  CCIfType<[v1i1, i8, i16], CCPromoteToType<i32>>,
   CCIfType<[i1, i8, i16], CCPromoteToType<i32>>,
 
----------------
Isn't the original line redundant?


================
Comment at: lib/Target/X86/X86CallingConv.td:592
 
   // Promote i1/i8/i16 arguments to i32.
+  CCIfType<[v1i1, i8, i16], CCPromoteToType<i32>>,
----------------
Update comment


================
Comment at: lib/Target/X86/X86CallingConv.td:594
+  CCIfType<[v1i1, i8, i16], CCPromoteToType<i32>>,
   CCIfType<[i1, i8, i16], CCPromoteToType<i32>>,
 
----------------
Isn't the old line here redundant?


================
Comment at: lib/Target/X86/X86CallingConv.td:805
 def CC_X86_32_C : CallingConv<[
   // Promote i1/i8/i16 arguments to i32.
+  CCIfType<[v1i1, i8, i16], CCPromoteToType<i32>>,
----------------
Fix the comment


================
Comment at: lib/Target/X86/X86CallingConv.td:807
+  CCIfType<[v1i1, i8, i16], CCPromoteToType<i32>>,
   CCIfType<[i1, i8, i16], CCPromoteToType<i32>>,
 
----------------
Isn't the second line here redundant?


================
Comment at: lib/Target/X86/X86CallingConv.td:827
   // Promote i1/i8/i16 arguments to i32.
+  CCIfType<[v1i1, i8, i16], CCPromoteToType<i32>>,
   CCIfType<[i1, i8, i16], CCPromoteToType<i32>>,
----------------
Merge with the line below and fix comment


================
Comment at: lib/Target/X86/X86CallingConv.td:840
   // Promote i1/i8/i16 arguments to i32.
+  CCIfType<[v1i1, i8, i16], CCPromoteToType<i32>>,
   CCIfType<[i1, i8, i16], CCPromoteToType<i32>>,
----------------
Merge with the line below and fix comment


================
Comment at: lib/Target/X86/X86CallingConv.td:871
   // Promote i1/i8/i16 arguments to i32.
+  CCIfType<[v1i1, i8, i16], CCPromoteToType<i32>>,
   CCIfType<[i1, i8, i16], CCPromoteToType<i32>>,
----------------
Merge with the line below and fix comment


================
Comment at: lib/Target/X86/X86CallingConv.td:879
   // Promote i1/i8/i16 arguments to i32.
+  CCIfType<[v1i1, i8, i16], CCPromoteToType<i32>>,
   CCIfType<[i1, i8, i16], CCPromoteToType<i32>>,
----------------
Merge with the line below and fix comment.


================
Comment at: lib/Target/X86/X86CallingConv.td:900
   // Promote i1/i8/i16 arguments to i32.
+  CCIfType<[v1i1, i8, i16], CCPromoteToType<i32>>,
   CCIfType<[i1, i8, i16], CCPromoteToType<i32>>,
----------------
Merge with the line below and fix comment


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:6990
       SplatIdx = idx;
-    else if (In != Op.getOperand(SplatIdx))
+    else if (IsSplat && In != Op.getOperand(SplatIdx))
       IsSplat = false;
----------------
Why did this if statement need to change?


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:14037
 
-  if (Op.getSimpleValueType() == MVT::i1)
+  if (VecVT.getVectorElementType() == MVT::i1)
     return ExtractBitFromMaskVector(Op, DAG);
----------------
What's the different between this and the original line? Doesn't Op's VT have to match the elements of the vector?


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:19127
+  }
+  if (Op.getOpcode() == X86ISD::VFPCLASSS) {
     return DAG.getNode(ISD::OR, dl, VT, Op, IMask);
----------------
This seems like an unrelated cleanup. Commit separately?

Why are curly braces being added?


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:29618
+    SDValue CondNew =
+        DAG.getNode(ISD::XOR, DL, Cond.getValueType(), Cond,
+                    DAG.getConstant(APInt::getAllOnesValue(8), DL, CondVT));
----------------
Can we reuse CondVT here instead of calling Cond.getValueType() again?

Why cant' we use DAG.getAllOnesConstant anymore? Does the constant have different with than CondVT's element type now? What does that do?


================
Comment at: lib/Target/X86/X86InstrAVX512.td:2303
+  defm : operation_gpr_mask_copy_lowering<VK8,  v8i1>;
+  defm : operation_gpr_mask_copy_lowering<VK16,  v16i1>;
+  defm : operation_gpr_mask_copy_lowering<VK32,  v32i1>;
----------------
Align v16i1 to same column as v8i1 above.


================
Comment at: lib/Target/X86/X86InstrAVX512.td:2316
+                                            GR8:$src, sub_8bit), (i32 1))),
+          VK16)>;
+  def : Pat<(X86kshiftr  (X86kshiftl (v8i1 (scalar_to_vector GR8:$src)), (i8 15)), (i8 15)) ,
----------------
The identation on the VK16 seems off. Same with VK8 on the pattern below.


================
Comment at: lib/Target/X86/X86InstrInfo.cpp:6333
 
-
   // SrcReg(VR128) -> DestReg(GR64)
   // SrcReg(VR64)  -> DestReg(GR64)
----------------
Not sure why there are two blanks lines in the current code, but don't delete the extra one in this patch.


https://reviews.llvm.org/D32273