[PATCH] D113414: [X86] Custom lowering of llvm.is_fpclass for x86_fp80

[Serge Pavlov via Phabricator via llvm-commits]

sepavloff added inline comments.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:22704
+
+  if (Check == ISD::fcNan) {
+    // If exceptions are ignored, use unordered comparison. It treats
----------------
pengfei wrote:
> This should also help for nan combined with other flags.
It looks faster to execute `FXAM` and then analyze its result in integer pipeline than doing checks in x87 registers and move the results to integer registers for analysis.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:22712-22725
+  unsigned Opc;
+  switch (ArgVT.SimpleTy) {
+  default:
+    llvm_unreachable("Unexpected type!");
+  case MVT::f32:
+    Opc = X86::XAM_Fp32;
+    break;
----------------
pengfei wrote:
> Can we assert the type only be `f80`?
Done just at the start of the function.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:721
     setOperationAction(ISD::LLRINT, MVT::f80, Custom);
+    setOperationAction(ISD::IS_FPCLASS, MVT::f80, Custom);
 
----------------
pengfei wrote:
> sepavloff wrote:
> > pengfei wrote:
> > > Add f32/f64 too?
> > There is concern that FXAM can be slow (https://reviews.llvm.org/D104853#2839746). So it is used only for f80, where x87 anyway is used. For other types a default lowering is used, which use usual float operations or integer arithmetic to make the tests. The resulted code for x86 is presented in D112025 in the test.
> Given that and we still need to distinguish sNaN with qNaN. Why don't we use integer arithmetic for `f80` too? We just need to check the high 32 bits for all classes expect fcZero and fcInf.
There is no `i80`, so doing integer arithmetic on `f80` is complicated. Existing default lowering simply crashes on `f80`. Also for many classes, like `fcZero`, `fcSubnormal`, `fcInf` or `fcSNan` we need to analyze all three words of `f80`. One instruction `FXAM` does all the job, and seems faster even if its latency is high.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:22762
+    // isfinite == !(isnan || isinf)
+  case ISD::fcInf | ISD::fcNan:
+    // isnan == (C3 == 0, C2 == 0, C0 == 1)
----------------
pengfei wrote:
> sepavloff wrote:
> > pengfei wrote:
> > > Can we define it with a new name? This give an impression the check conditions can be permuted. But the code seems don't support it.
> > Actually the checks can be permuted. I reformatted the comments, hopefully they make this code clearer.
> But the switch doesn't enumerate all possibilities, e.g., ISD::fcZero | ISD::fcInf. I don't think silently break for these cases are correct.
This switch only processes some combinations that are simple to implement. For all other combinations the general case is applied.

This patch was tested using runtime tests from D112933. They contains all combinations of two basic tests and many other combinations.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:22848
+      // Check the quiet NaN bit.
+      APInt QNaNBitMask(32, 0x40000000);
+      SDValue QNaNBitMaskV = DAG.getConstant(QNaNBitMask, DL, MVT::i32);
----------------
pengfei wrote:
> sepavloff wrote:
> > pengfei wrote:
> > > This only works for fp80. We should check the type first.
> > > Can we check exception #IA for f32 and f64?
> > Fixed.
> I don't understand. If we only customize for `f80`. Why do we always add code for `f32` and `f64`?
There was some hesitation whether using `FXAM` could be used for types other than `f80` in some cases. Now support of these types is removed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113414