[PATCH] D13632: AVX512 : i8/i16 vector CTLZ/CTLZ_ZERO_UNDEF lowering

[Simon Pilgrim via llvm-commits]

RKSimon added a comment.

Thanks for looking at this - minor comments below. Elena should review the AVX512 internals.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:1522
@@ -1516,2 +1521,3 @@
+      setOperationAction(ISD::CTLZ_ZERO_UNDEF,  MVT::v32i8,  Custom);
 
       setOperationAction(ISD::CTTZ_ZERO_UNDEF,  MVT::v8i64, Custom);
----------------
There is a mix of 128-bit, 256-bit and 512-bit types here - probably best to separate them (but still under the same hasCDI() test) and add a comment on what is going on.

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:1537
@@ -1529,2 +1536,3 @@
+      setOperationAction(ISD::CTLZ_ZERO_UNDEF,  MVT::v8i16, Custom);
 
       setOperationAction(ISD::CTTZ_ZERO_UNDEF,  MVT::v4i64, Custom);
----------------
Is v16i8 not possible here?

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:17387
@@ +17386,3 @@
+  assert((EltVT == MVT::i8 || EltVT == MVT::i16) &&
+          "Unsupported element type");
+
----------------
Please can you add a minimal amount of hasAVX512() protection to the function (maybe as an early out)? Its likely that someone will get around to adding SSSE3 style vector CTLZ lowering at some point and would want to reuse some of the function's setup.

Alternatively rename this function LowerVectorCTLZ_AVX512 and add a hasAVX512() test to where it is called?

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:17390
@@ +17389,3 @@
+  if (512 < (NumElems * 32)) {
+  // split vector , it's L0 and Hi parts will be handled in next iteration
+    SDValue Lo, Hi;
----------------
Fix style + grammar - correct the indentation and add a full stop.

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:17405
@@ +17404,3 @@
+          "Unsupported value type for operation");
+
+  // use native supported vector instruction vplzcntd
----------------
Is VT.is512BitVector() actually supported here? Surely when you zero extend the vector will exceed 512 bits? Maybe a second assert testing that (NewVT.is256BitVector() || NewVT.is512BitVector())?

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:17406
@@ +17405,3 @@
+
+  // use native supported vector instruction vplzcntd
+  Op = DAG.getNode(ISD::ZERO_EXTEND, dl, NewVT, Op.getOperand(0));
----------------
Fix style + grammar

================
Comment at: test/CodeGen/X86/vector-lzcnt-512.ll:1
@@ -1,1 +1,2 @@
 ; RUN: llc < %s -mtriple=x86_64-apple-darwin -mcpu=knl -mattr=+avx512cd | FileCheck %s --check-prefix=ALL --check-prefix=AVX512 --check-prefix=AVX512CD
+; RUN: llc < %s -mtriple=x86_64-apple-darwin -mcpu=skx | FileCheck %s --check-prefix=SKX
----------------
These prefixes probably need tidying up?


Repository:
  rL LLVM

http://reviews.llvm.org/D13632