[PATCH] D116702: [GlobalISel] Combine select + fcmp to fminnum/fmaxnum/fminimum/fmaximum

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:5376-5379
+    if (isLegalOrBeforeLegalizer({TargetOpcode::G_FMINNUM, {DstTy}}))
+      return TargetOpcode::G_FMINNUM;
+    else if (isLegalOrBeforeLegalizer({TargetOpcode::G_FMINIMUM, {DstTy}}))
+      return TargetOpcode::G_FMINIMUM;
----------------
paquette wrote:
> aemerson wrote:
> > What's the reason for choosing this particular ordering of legality checks such that we prefer G_FMINNUM?
>  I was just matching the behaviour in SelectionDAGBuilder so I'm not entirely sure:
> 
> ```
>       case SPNB_RETURNS_ANY: {
>         if (TLI.isOperationLegalOrCustom(ISD::FMINNUM, VT))
>           Opc = ISD::FMINNUM;
>         else if (TLI.isOperationLegalOrCustom(ISD::FMINIMUM, VT))
>           Opc = ISD::FMINIMUM;
> ```
> 
> Maybe @arsenm knows better since I think he wrote the original code. (https://reviews.llvm.org/rGfabab4b7dd7d4ccefec2bb6cd405044429637ba6)
FMINIMUM/FMAXIMUM are not widely used/available. The DAG version is more strict in checking for isLegalOrCustom, whereas here you're forming them before legalize anyway. 

I think this should follow that and look for some positive legality only. The trickier case I'm worried about is on vector types which will be split later. We probably should have a utility that looks like the getTypeAction loop for finding the ultimate legalized type


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

https://reviews.llvm.org/D116702