[PATCH] D91716: AMDGPU/GlobalISel: Calculate isKnownNeverNaN for fminnum and fmaxnum

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/test/CodeGen/AMDGPU/GlobalISel/legalize-fminnum.mir:773
     %3:_(s32) = COPY $vgpr2
     %4:_(s32) = G_FMINNUM %2, %3
     $vgpr0 = COPY %4
----------------
Petar.Avramovic wrote:
> arsenm wrote:
> > Petar.Avramovic wrote:
> > > arsenm wrote:
> > > > Petar.Avramovic wrote:
> > > > > 
> > > > > 
> > > > > 
> > > > > ```
> > > > > return isKnownNeverNaN(Op.getOperand(0), SNaN, Depth + 1) ||
> > > > >        isKnownNeverNaN(Op.getOperand(1), SNaN, Depth + 1);
> > > > > }
> > > > > ```
> > > > > This is legalized first. `%2` is isKnownNeverSNaN with IEEE = true, but since inputs for `%2:_(s32) = G_FMAXNUM %0, %1` are not yet canonicalized isKnownNeverSNaN will fail and canonicalize is inserted. However since we know what will happen with `'%2:_(s32) = G_FMAXNUM %0, %1` we declare it  isKnownNeverSNaN to not depend on order of legalization.
> > > > > 
> > > > > 
> > > > The legalization order doesn't matter. These operations have their own independent semantics. isKnownNeverNaN needs to understand both pairs
> > > How can this be done?
> > > >The legalization order doesn't matter.
> > > Formula above gives different result depending if input was legalized already or not. It would work if we would legalize uses first. 
> > The same way SelectionDAG handles this:
> > 
> > ```
> >   case ISD::FMINNUM_IEEE:
> >   case ISD::FMAXNUM_IEEE: {
> >     if (SNaN)
> >       return true;
> >     // This can return a NaN if either operand is an sNaN, or if both operands
> >     // are NaN.
> >     return (isKnownNeverNaN(Op.getOperand(0), false, Depth + 1) &&
> >             isKnownNeverSNaN(Op.getOperand(1), Depth + 1)) ||
> >            (isKnownNeverNaN(Op.getOperand(1), false, Depth + 1) &&
> >             isKnownNeverSNaN(Op.getOperand(0), Depth + 1));
> >   }
> > ```
> I meant for `FMINNUM`, writing same code as in SDAG doesn't deal with unnecessary canonicalize. `FMINNUM_IEEE` is the same (there is only `if (SNaN)` part atm).
The lowering for fminnum to fminnum_ieee tries to avoid the canonicalize depending on the instruction/inputs. This is how SelectionDAG also does it. For this patch, it just needs to fully handle the correct semanics for the two pairs


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

https://reviews.llvm.org/D91716