Page MenuHomePhabricator

[FPEnv][X86] Implement lowering of llvm.set.rounding
Needs ReviewPublic

Authored by sepavloff on Feb 17 2020, 10:26 AM.

Diff Detail

Event Timeline

sepavloff created this revision.Feb 17 2020, 10:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 17 2020, 10:26 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
craig.topper added inline comments.Feb 17 2020, 10:50 AM
llvm/lib/Target/X86/X86ISelLowering.cpp
26992

Why do we need an APInt?

27061

Why do we need an APInt?

27075

Removed commented out code.

31334

Where did FNSTSW16r come from? I deleted that recently.

llvm/lib/Target/X86/X86ISelLowering.h
29

STMXCSR/LDMXCSR aren't used.

craig.topper added inline comments.Feb 17 2020, 11:14 AM
llvm/lib/Target/X86/X86ISelLowering.cpp
26976

I have no idea why FLT_ROUNDS calls TFI.getStackAlignment() to get the second parameter here. It should just be 4.

Modified patch according to reviewer's notes

sepavloff marked 7 inline comments as done.Feb 18 2020, 12:00 AM
sepavloff added inline comments.
llvm/lib/Target/X86/X86ISelLowering.cpp
26976

Changed here but implementation of LowerFLT_ROUNDS_ was not changed.

RKSimon added inline comments.Feb 18 2020, 6:45 AM
llvm/lib/Target/X86/X86ISelLowering.cpp
31334

FNSTSW16r was removed at D73863, but FNSTCW16m stayed

@sepavloff - what's happening with this?

sepavloff updated this revision to Diff 260283.Apr 27 2020, 4:55 AM
sepavloff marked an inline comment as done.

Rebased patch

@sepavloff - what's happening with this?

It is still needed. This patch depends on D74729. Next steps, like support in frontend (as fesetround) and lowering on other platforms make sense only after destiny of these patches will be determined.

The motivation for this work is implementation of a pragma that would set/restore rounding mode without need to call fesetround.

kpn added a subscriber: kpn.Apr 27 2020, 10:32 AM

Rebased patch

RKSimon added inline comments.Aug 11 2020, 12:56 PM
llvm/test/CodeGen/X86/fpenv.ll
5

regenerate with the update_llc_test_checks.py script?

sepavloff updated this revision to Diff 285115.Aug 12 2020, 9:33 AM

Rebased and used update_llc_test_checks.py script

sepavloff updated this revision to Diff 285121.Aug 12 2020, 9:38 AM

Clang-formatted the patch

sepavloff updated this revision to Diff 320677.Feb 1 2021, 10:28 PM

Rebased patch

sepavloff updated this revision to Diff 327080.Mar 1 2021, 4:18 AM

Rebased patch

lebedev.ri added inline comments.
llvm/lib/Target/X86/X86ISelLowering.cpp
27007–27008

Is this enforced elsewhere? (verifier, etc)
This doesn't seem like the right way do to error handling to me, we shouldn't crash.

llvm/lib/Target/X86/X86ISelLowering.h
855–856

I suspect this is backwards, and should say that glibc choose to use values compatible with the ones stored in FPSR.

sepavloff updated this revision to Diff 327377.Mar 2 2021, 12:53 AM

Updated comment

sepavloff added inline comments.Mar 2 2021, 12:55 AM
llvm/lib/Target/X86/X86ISelLowering.cpp
27007–27008

It is not an error handling, it is an assertion check. This intrinsic is a low-level entity and is actually only a wrap over an instruction that writes to control register. It is responsibility of a caller to ensure that argument of this intrinsic is valid. A check in verifier is possible but is not much helpful, because the argument may be a runtime expression.

llvm/lib/Target/X86/X86ISelLowering.h
855–856

Updated the comment.

Is there anything I can do to facilitate the review process?

pengfei added inline comments.
llvm/lib/Target/X86/X86ISelLowering.cpp
26985

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?

sepavloff updated this revision to Diff 338898.Tue, Apr 20, 9:18 AM

Lower set_rounding only if soft-float is off

sepavloff added inline comments.Tue, Apr 20, 9:48 AM
llvm/lib/Target/X86/X86ISelLowering.cpp
26985

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.

pengfei added inline comments.Thu, Apr 22, 12:58 AM
llvm/lib/Target/X86/X86ISelLowering.cpp
26985

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

sepavloff updated this revision to Diff 339628.Thu, Apr 22, 7:41 AM

Added check for Subtarget.hasX87() to support -mno-80387

sepavloff added inline comments.Thu, Apr 22, 7:47 AM
llvm/lib/Target/X86/X86ISelLowering.cpp
26985

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.

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

llvm/lib/Target/X86/X86ISelLowering.cpp
26985

Makes sense to me, thanks.

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

@pengfei Thank you!

Could anybody sign off this patch?

I have no objections to this going in - @pengfei @craig.topper @andrew.w.kaylor ?