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

[Petar Avramovic via Phabricator via llvm-commits]

Petar.Avramovic added inline comments.


================
Comment at: llvm/lib/CodeGen/GlobalISel/Utils.cpp:494-500
+    // Known NaN assumptions for G_FMINNUM and G_FMAXNUM might be affected by
+    // target specific options and lowering actions. False by default.
+    case TargetOpcode::G_FMINNUM:
+    case TargetOpcode::G_FMAXNUM: {
+      const TargetLowering *TLI = MF->getSubtarget().getTargetLowering();
+      return TLI->isKnownNeverNaNForMI(Val, MRI, SNaN);
+    }
----------------
arsenm wrote:
> Generic opcodes should not have target dependent semantics like this, especially since these already have semantics inherited from the IR/C fmin/fmax definition. They return the non-nan argument regardless of snan or not
> Generic opcodes should not have target dependent semantics like this, especially since these already have semantics inherited from the IR/C fmin/fmax definition.
I would say that only "is Known Never any NaN" is given. This patch does not change that (isKnownNeverNaN(SNaN = false)).
>They return the non-nan argument regardless of snan or not
still stands. We are just aware of the way we lower instruction and use it in isKnownNeverSNaN only. This is only used for efficiency to avoid having to quiet "known non SNaN" into instruction that is about to be lowered.



================
Comment at: llvm/lib/CodeGen/GlobalISel/Utils.cpp:498-499
+    case TargetOpcode::G_FMAXNUM: {
+      const TargetLowering *TLI = MF->getSubtarget().getTargetLowering();
+      return TLI->isKnownNeverNaNForMI(Val, MRI, SNaN);
+    }
----------------
arsenm wrote:
> Petar.Avramovic wrote:
> > Can this then be `return true`?
> No, it depends on the inputs. You can refer to the existing SelectionDAG::isKnownNeverNaN:
> 
> ```
>     // Only one needs to be known not-nan, since it will be returned if the
>     // other ends up being one.
>     return isKnownNeverNaN(Op.getOperand(0), SNaN, Depth + 1) ||
>            isKnownNeverNaN(Op.getOperand(1), SNaN, Depth + 1);
> 
> ```
I don't think 
```
    return isKnownNeverNaN(DefMI->getOperand(1).getReg(), MRI, SNaN) ||
           isKnownNeverNaN(DefMI->getOperand(2).getReg(), MRI, SNaN);
```
is enough for globalisel since it does not look at the whole picture. The formula above works when target naturally has FMINNUM instruction and knowing the inputs gives conclusion about the output (one input is not nan result is not nan). This would be generic formula (FIXME, use this formula in isKnownNeverNaNForMI instead of return false).
But subtarget with IEEE=true does not have FMINNUM and has to lower it to FMINNUM_IEEE. This gives additional critical piece of information FMINNUM is never SNaN with IEEE=true.
Are test changes correct?



================
Comment at: llvm/test/CodeGen/AMDGPU/GlobalISel/legalize-fminnum.mir:773
     %3:_(s32) = COPY $vgpr2
     %4:_(s32) = G_FMINNUM %2, %3
     $vgpr0 = COPY %4
----------------
arsenm wrote:
> 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
About examining the input: We are likely examining inputs in the middle of the legalization. With IEEE=true fminnum will never be the input but fminnum_ieee. Recursively asking for fminnum's inputs is not correct in this context since (maybe quieted)inputs will go to fminnum_ieee (its result is never SNaN) not fminnum and this is target dependent. 
I don't see how this breaks correct semantics. 


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

https://reviews.llvm.org/D91716