[PATCH] D14815: AVX-512: INSERT_SUBVECTOR optimization for i1 vectors

[Mitch Bodart via llvm-commits]

mbodart added a comment.

Hi Elena,

In general the changes LGTM, I had just a couple minor questions/comments.

- mitch


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:6571
@@ -6494,1 +6570,3 @@
   if (NumOfOperands > 2) {
+
+    unsigned NumOfDefinedOps = 0;
----------------
A simple one line comment here would be useful. something like:

// Specialize the cases when all, or all but one, of the operands are undef.

================
Comment at: lib/Target/X86/X86InstrAVX512.td:2422
@@ -2421,13 +2421,3 @@
 
-let Predicates = [HasVLX] in {
-  def : Pat<(v8i1 (insert_subvector undef, (v4i1 VK4:$src), (iPTR 0))),
-            (v8i1 (COPY_TO_REGCLASS VK4:$src, VK8))>;
-  def : Pat<(v8i1 (insert_subvector undef, (v2i1 VK2:$src), (iPTR 0))),
-            (v8i1 (COPY_TO_REGCLASS VK2:$src, VK8))>;
-  def : Pat<(v4i1 (insert_subvector undef, (v2i1 VK2:$src), (iPTR 0))),
-            (v4i1 (COPY_TO_REGCLASS VK2:$src, VK4))>;
-  def : Pat<(v4i1 (extract_subvector (v8i1 VK8:$src), (iPTR 0))),
-            (v4i1 (COPY_TO_REGCLASS VK8:$src, VK4))>;
-  def : Pat<(v2i1 (extract_subvector (v8i1 VK8:$src), (iPTR 0))),
-            (v2i1 (COPY_TO_REGCLASS VK8:$src, VK2))>;
-}
+def : Pat<(v4i1 (extract_subvector (v8i1 VK8:$src), (iPTR 0))),
+          (v4i1 (COPY_TO_REGCLASS VK8:$src, VK4))>;
----------------
What are the consequences of removing the HasVLX predicate?

Was it unnecessary in the first place, perhaps because the mere presence
of the VK* register classes already implied it?

================
Comment at: test/CodeGen/X86/avx512-skx-insert-subvec.ll:1
@@ +1,2 @@
+; RUN: llc < %s -march=x86-64 -mtriple=x86_64-apple-darwin -mcpu=skx | FileCheck %s
+
----------------
Isn't -march=x86-64 already implied by the triple?

In one of my earlier change sets, Michael Kuperstein recommended using -mattr instead of -mcpu as a general testing guideline.  I don't have a strong opinion either way, but it's something to consider.

Finally, can you add some tests for knl (i.e., vanilla avx512f)?


Repository:
  rL LLVM

http://reviews.llvm.org/D14815