[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);
 
----------------
sepavloff wrote:
> pengfei wrote:
> > sepavloff wrote:
> > > pengfei wrote:
> > > > sepavloff wrote:
> > > > > 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.
> > > > I mean we still customize `f80` here, but enrich the code for checking S/QNAN to more classes by loading bits [79:48] rather than [63:32]. We can easily check whether the remainding 48 bits are zero for `fcZero` and `fcInf`.
> > > > Not only for performance, but also we should lower it when `f80` is used without `x87` enabled. Although it's rare of such case, it's supported by compiler.
> > > > but also we should lower it when f80 is used without x87 enabled. Although it's rare of such case, it's supported by compiler.
> > > 
> > > How can this mode be activated? If I provide option `-mno-x87`, compiler fails in `TargetLoweringBase::getRegClassFor` in:
> > > ```
> > > assert(RC && "This value type is not natively supported!");
> > > ```
> > Yeah, Clang may still have some problem to handle such cases, but GCC does support it. https://godbolt.org/z/aza81fKx4
> > We shouldn't make it worse :)
> As I understand, it is what `-msoft-float` should do but it doesn't in clang. Things are already bad :)
> 
> If FP operations in this case are implemented by library functions, it must be easier, more efficient and reliable to implement a special function for this intrinsic, rather than emulate the FP operations in the compiler.
Adding a new library function is not an option, because we cannot add it to libgcc. The base patch (D112025) changed to support fp80 as well. Functions with suffix "soft" in the test file `is_fpclass-fp80.ll` demonstrate lowering for this case when x87 unit is not available.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113414