[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:721
     setOperationAction(ISD::LLRINT, MVT::f80, Custom);
+    setOperationAction(ISD::IS_FPCLASS, MVT::f80, Custom);
 
----------------
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.


================
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);
----------------
pengfei wrote:
> Why only f80?
You are right, this check is not needed. Removed.


================
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:
> 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.


================
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;
----------------
pengfei wrote:
> Define it in `FPSWFlag` ?
I would like but I cannot invent a good name for it. Added comment to describe why this constant is used.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:22808
+                        IsInverted ? ISD::SETNE : ISD::SETEQ);
+  assert(!CCToCompare);
+
----------------
pengfei wrote:
> It's equal to assert at line 22751 and better there with message.
The assert only checked that for each `CCToCompare` there is corresponding `ExpectedCC`. There are not many cases here, it seems that this check is safe to remove.


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


================
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
----------------
pengfei wrote:
> Where's fucomp generated from? I didn't see it in the code.
It appears as a result of the call `DAG.getSetCC(DL, ResultVT, Arg, Arg, ISD::CondCode::SETUNE)`, which is called at the beginning of `lowerIS_FPCLASS` if exceptions are ignored.


================
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
----------------
pengfei wrote:
> Why `fucompi`?
It is also the result of `getSetCC`. This code must be generated for unordered comparison.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113414