[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:721
     setOperationAction(ISD::LLRINT, MVT::f80, Custom);
+    setOperationAction(ISD::IS_FPCLASS, MVT::f80, Custom);
 
----------------
Add f32/f64 too?


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:22707
+    // unsupported values as NaNs, which is compatible with glibc.
+    if (ArgVT == MVT::f80 && Op->getFlags().hasNoFPExcept())
+      return DAG.getSetCC(DL, ResultVT, Arg, Arg, ISD::CondCode::SETUNE);
----------------
Why only f80?


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


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:22765
+    // isinf == (C3 == 0, C2 == 1, C0 == 1)
+  case ISD::fcZero | ISD::fcSubnormal:
+    // iszero == (C3 == 1, C2 == 0, C0 == 0)
----------------
ditto.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:22769
+    CCToCompare = DAG.getNode(ISD::AND, DL, MVT::i16, FPSW,
+                              DAG.getConstant(X86::C3 | X86::C0, DL, MVT::i16));
+    break;
----------------
Define it in `FPSWFlag` ?


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:22808
+                        IsInverted ? ISD::SETNE : ISD::SETEQ);
+  assert(!CCToCompare);
+
----------------
It's equal to assert at line 22751 and better there with message.


================
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);
----------------
This only works for fp80. We should check the type first.
Can we check exception #IA for f32 and f64?


================
Comment at: llvm/test/CodeGen/X86/x86-is_fpclass-fp80.ll:9
+; CHECK-32-NEXT:    fldt {{[0-9]+}}(%esp)
+; CHECK-32-NEXT:    fucomp %st(0)
+; CHECK-32-NEXT:    fnstsw %ax
----------------
Where's fucomp generated from? I didn't see it in the code.


================
Comment at: llvm/test/CodeGen/X86/x86-is_fpclass-fp80.ll:21
+; CHECK-64-NEXT:    fldt {{[0-9]+}}(%rsp)
+; CHECK-64-NEXT:    fucompi %st(0), %st
+; CHECK-64-NEXT:    setp %cl
----------------
Why `fucompi`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113414