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

[Igor Breger via Phabricator via llvm-commits]

igorb added inline comments.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:5106
 
+  // If this node widens - by concatenating zeroes - the type of the result
+  // of a node with instruction that zeroes all upper (irrelevant) bits of the
----------------
I think that the comment is unclear.
Please move the implementation after comments "  // There are 3 possible cases: "  line 5123


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:5110
+  // the v8i1 version of the previous instruction during instruction selection.
+  SDValue Inserted = Op.getOperand(1);
+  if (isMaskedZeroUpperBitsvXi1(Inserted.getOpcode()) ||
----------------
please see line:5086


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:5111
+  SDValue Inserted = Op.getOperand(1);
+  if (isMaskedZeroUpperBitsvXi1(Inserted.getOpcode()) ||
+      (Inserted.getOpcode() == ISD::AND &&
----------------
Hi , 
This is incorrect, you should also check ISD::isBuildVectorAllZeros(Vec.getNode()) .
please add test case also.



================
Comment at: lib/Target/X86/X86ISelLowering.cpp:7907
+// k-register.
+static SDValue isTypePromotionOfi1ZeroUpBits(SDValue Op) {
+  unsigned Opc = Op.getOpcode();
----------------
please check if you can use void SelectionDAG::computeKnownBits  and update 
void X86TargetLowering::computeKnownBitsForTargetNode   instead



https://reviews.llvm.org/D33188