[PATCH] D119111: [X86] Invert a vector select IR canonicalization with a binop identity constant

[LuoYuanke via Phabricator via llvm-commits]

LuoYuanke added inline comments.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:48950
+      case ISD::FMUL: // X * 1.0 --> X
+      case ISD::FDIV: // X * 1.0 --> X
+        return C->isExactlyValue(1.0);
----------------
pengfei wrote:
> `/`
Sorry, I don't understand this comment.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:49012-49015
+  if (SDValue Sel = combineBinopWithSelect(N, DAG, Subtarget))
+    return Sel;
+
+  return SDValue();
----------------
pengfei wrote:
> Equals to `return combineBinopWithSelect(N, DAG, Subtarget)`?
OK, I'll update it.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:53889
   case ISD::FSUB:           return combineFaddFsub(N, DAG, Subtarget);
+  case ISD::FMUL:           return combineFmul(N, DAG, Subtarget);
+  case ISD::FDIV:           return combineFdiv(N, DAG, Subtarget);
----------------
pengfei wrote:
> Or call `combineBinopWithSelect` directly?
I prefer to following the current coding convention, so that when there is more to combine we can extent the code in the sub-function.


================
Comment at: llvm/test/CodeGen/X86/vector-bo-select.ll:345-346
 ; AVX512-NEXT:    vptestmd %zmm0, %zmm0, %k1
-; AVX512-NEXT:    vbroadcastss {{\.?LCPI[0-9]+_[0-9]+}}(%rip), %zmm2 {%k1}
 ; AVX512-NEXT:    vmulps %zmm2, %zmm1, %zmm0
+; AVX512-NEXT:    vmovaps %zmm1, %zmm0 {%k1}
 ; AVX512-NEXT:    retq
----------------
pengfei wrote:
> Curiously: it should be equal to
> ```
> vmulps %zmm2, %zmm1, %zmm1 {%k1}
> vmovaps %zmm1, %zmm0
> ```
> Why sometimes we use this way, sometime another?
I guess it is because sometime it is commuted or swapped, sometimes it is not.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119111/new/

https://reviews.llvm.org/D119111