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

[Phoebe Wang via Phabricator via llvm-commits]

pengfei added inline comments.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:53215
 
+static SDValue combineFMinimumFMaximum(SDNode *N, SelectionDAG &DAG,
+                                       const X86Subtarget &Subtarget) {
----------------
pengfei wrote:
> e-kud wrote:
> > 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`?
> No, not the name. Did you try set action of `FMAXIMUM` to `Custom`? But I'm fine given it's similar to `combineFMinNumFMaxNum`.
Answer my previous question: `combineFMinNumFMaxNum` was intended here due to the reason described in D15294. I suppose the problem doesn't exist to `combineFMinimumFMaximum`. So I prefer to `LowerFMinimumFMaximum`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145634