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

[Demikhovsky, Elena via llvm-commits]

Mitch,

Thank you for the review. I applied your comments.
> What are the consequences of removing the HasVLX predicate?
VK legal types cover predicates. (If VK4 of VK2 are legal, the VLX is on)

 > Finally, can you add some tests for knl
It is not "interesting", illegal types will be promoted or split. The code will be ugly.

-  Elena

-----Original Message-----
From: Mitch Bodart [mailto:mitch.l.bodart at intel.com] 
Sent: Friday, November 20, 2015 22:33
To: Demikhovsky, Elena <elena.demikhovsky at intel.com>; Breger, Igor <igor.breger at intel.com>; Bodart, Mitch L <mitch.l.bodart at intel.com>
Cc: llvm-commits at lists.llvm.org
Subject: Re: [PATCH] D14815: AVX-512: INSERT_SUBVECTOR optimization for i1 vectors

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



---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.