[PATCH] D33188: [X86][AVX512] Improve lowering of AVX512 compare intrinsics (remove redundant shift left+right instructions).

[Ayman Musa via Phabricator via llvm-commits]

aymanmus added inline comments.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:5069
+// destination and accepts mask.
+static bool isMaskedZeroUpperBitsvXi1(unsigned int Opcode) {
+  switch (Opcode) {
----------------
zvi wrote:
> If think you should check here for AND
True should be returned only if the instruction zeros the upper bits of the destination k-register and can be masked.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:7893
+// except for the first one. (CONCAT_VECTORS Op, 0, 0,...,0)
+static bool isExpandWithZeros(const SDValue &Op) {
+  assert(Op.getOpcode() == ISD::CONCAT_VECTORS &&
----------------
zvi wrote:
> Can all these helpers return SDValue() instead of false? That would save the need to pass and modify the reference argument.
isExpandWithZeros doesn't modify the SDValue, but for isTypePromotionOfi1ZeroUpBits it is indeed better to change it.
Thanks.


================
Comment at: lib/Target/X86/X86InstrAVX512.td:3031
 
+let Predicates = [HasAVX512, NoVLX] in {
+def : Pat<(v8i1 (X86pcmpgtm (v8i32 VR256X:$src1), (v8i32 VR256X:$src2))),
----------------
zvi wrote:
> Are all these covered by tests?
Adding knl run for this (with checks on relevant test cases).
Thanks


https://reviews.llvm.org/D33188