[PATCH] D145634: [X86] Support llvm.{min,max}imum.f{32,64}

[Evgenii Kudriashov via Phabricator via llvm-commits]

e-kud added a reviewer: RKSimon.
e-kud marked 3 inline comments as done.
e-kud added inline comments.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:53215
 
+static SDValue combineFMinimumFMaximum(SDNode *N, SelectionDAG &DAG,
+                                       const X86Subtarget &Subtarget) {
----------------
pengfei wrote:
> The code doesn't combine anything, should be moved to `LowerFMinimumFMaximum`?
I've tried to find a better place to handle `ISD::FMAXIMUM` but there is no place in chain Selecting->Combining->Legalization. So, I was inspired by `ISD::FMAXNUM` that is actually lowered during combining.

If you are about naming solely, it is not a big deal to change the name, but all callees in `PerformDAGCombine` are named as `combine*` even for `ISD::FMAXNUM`. Do we really want to have a single `LowerFMinimumFMaximum`?


================
Comment at: llvm/test/CodeGen/X86/fminimum-fmaximum.ll:181
+; SSE2:       # %bb.0:
+; SSE2-NEXT:    movsd {{.*#+}} xmm0 = mem[0],zero
+; SSE2-NEXT:    retq
----------------
pengfei wrote:
> pengfei wrote:
> > The test does show anything interesting.
> does -> doesn't
Probably, yes. I wanted to show that we are able to fold all these checks with constant arguments. But we've already tested folding of zero and nan checks. Original FMAX and FMIN are tested as well. Dropped it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145634