[PATCH] D38732: [X86][AVX512] Improve lowering of AVX512 test intrinsics

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added inline comments.


================
Comment at: lib/Target/X86/X86ISelDAGToDAG.cpp:457
+      N->getOpcode() == X86ISD::TESTNM ||
       N->getOpcode() == X86ISD::CMPMU) {
     // We can get 256-bit 8 element types here without VLX being enabled. When
----------------
RKSimon wrote:
> Pull out repeated N->getOpcode()
> ```
> unsigned Opcode = N->getOpcode();
> if (Opcode == X86ISD::PCMPEQM || Opcode == X86ISD::PCMPGTM ||
>     Opcode == X86ISD::TESTM || Opcode == X86ISD::TESTNM ||
>     Opcode == X86ISD::CMPM || Opcode == X86ISD::CMPMU) {
> ```
Cheers, you should be able to clang-format this on to fewer lines


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:17188
+    SDValue A = peekThroughBitcasts(Op0);
+    SDValue B = peekThroughBitcasts(Op1);
+    APInt C;
----------------
craig.topper wrote:
> isBuildVectorAllZeros already peeks through bitcasts so you can just check on Op1 directly.
Do we need to support X86ISD::FAND as well?


================
Comment at: test/CodeGen/X86/setcc-lowering.ll:28
+; KNL-32-NEXT:    vbroadcastss {{.*#+}} ymm1 = [8388607,8388607,8388607,8388607,8388607,8388607,8388607,8388607]
+; KNL-32-NEXT:    vptestnmd %zmm1, %zmm0, %k0
 ; KNL-32-NEXT:    movb $15, %al
----------------
Should the vptest instructions be in the Integer domain? That should force a vpbroadcastd.

Kind of a pity that the broadcast doesn't fold, but since you're messing with subregs it's not that surprising.


https://reviews.llvm.org/D38732