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

[Phoebe Wang via Phabricator via llvm-commits]

pengfei 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
----------------
This should also help for nan combined with other flags.


================
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;
----------------
Can we assert the type only be `f80`?


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:721
     setOperationAction(ISD::LLRINT, MVT::f80, Custom);
+    setOperationAction(ISD::IS_FPCLASS, MVT::f80, Custom);
 
----------------
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.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:22762
+    // isfinite == !(isnan || isinf)
+  case ISD::fcInf | ISD::fcNan:
+    // isnan == (C3 == 0, C2 == 0, C0 == 1)
----------------
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.


================
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);
----------------
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`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113414