[PATCH] D74730: [FPEnv][X86] Implement lowering of llvm.set.rounding

[Pengfei Wang via Phabricator via llvm-commits]

pengfei added a comment.

This patch LGTM, but I'd like other sign off.



================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:26949
+
+  // Store FP control word into memory.
+  SDValue Ops[] = {Chain, StackSlot};
----------------
sepavloff wrote:
> pengfei wrote:
> > sepavloff wrote:
> > > pengfei wrote:
> > > > I think we cannot assume x87 is always available https://godbolt.org/z/4MbsarMcP. GCC supports nox87 feature, @LiuChen3 is doing the same thing in LLVM in D100091.
> > > > Furthermore, do we need to add a function call in case softfloat/libcall is used? Can we wisely generate one of them based on situation?
> > > > I think we cannot assume x87 is always available https://godbolt.org/z/4MbsarMcP. GCC supports nox87 feature, @LiuChen3 is doing the same thing in LLVM in D100091.
> > > 
> > > Is there an x86 core that does not support x87 instructions? If no, then `-mno-80387`, similar to `-msoft-float` and `-mgeneral-regs-only` are proposed for the cases like Linux kernel, where access to FP hardware is undesirable because the modification of FP state requires saving/restoring it in process context. As `set_rounding` by essence modifies FP state, `-mno-80387` is identical to `-msoft-float`. By the way, GCC documentation list them as synonyms (https://gcc.gnu.org/onlinedocs/gcc/x86-Options.html).
> > > 
> > > > Furthermore, do we need to add a function call in case softfloat/libcall is used? Can we wisely generate one of them based on situation?
> > > 
> > > I added checks for soft-float in constructor of `X86TargetLowering`. If this option is effect, this function is not used for lowering of `set_rounding`.
> > > 
> > > 
> > I think Core and most Atom support x87 instructions. Someone mentioned there're some targets for IoT don't. https://retrocomputing.stackexchange.com/a/9662
> Interesting reference, thank you.
> 
> It is still true that if hardware supports FP operations, it supports x87 also. There is no a processor that supports FP in hardware but do not support x87. So we do not need to check for x87 presence here, it should be enough to enable or not custom lowering in the constructor of `X86TargetLowering`. I added the check for `Subtarget.hasX87()` to support `-mno-80387` also.
Makes sense to me, thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74730