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

[Amara Emerson via Phabricator via llvm-commits]

aemerson added a comment.

Second reverse ping.



================
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;
----------------
arsenm wrote:
> 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
Since this is handling RETURNS_ANY before legalization seems fine to me?


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

https://reviews.llvm.org/D116702