Page MenuHomePhabricator

[RISCV] Support fe_getround and fe_raise_inexact in builtins
ClosedPublic

Authored by StephenFan on Jun 20 2022, 8:34 PM.

Diff Detail

Event Timeline

StephenFan created this revision.Jun 20 2022, 8:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 20 2022, 8:34 PM
StephenFan requested review of this revision.Jun 20 2022, 8:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 20 2022, 8:34 PM
Herald added subscribers: Restricted Project, pcwang-thead, eopXD. · View Herald Transcript
luismarques added inline comments.Jun 22 2022, 4:50 AM
compiler-rt/lib/builtins/riscv/fp_mode.c
41–42

Can't this be replaced with a single csrrs?

luismarques added inline comments.Jun 22 2022, 4:56 AM
compiler-rt/lib/builtins/riscv/fp_mode.c
41–42

More specifically, csrrsi, as RISCV_INEXACT fits the 5-bit immediate.

StephenFan added inline comments.Jun 29 2022, 8:40 AM
compiler-rt/lib/builtins/riscv/fp_mode.c
41–42

I am a beginner at inline assembly programming. The statement flags | RISCV_INEXACT's value is only known at runtime instead of compile-time, can we still use csrrsi instruction to set the float flags?

jrtc27 added inline comments.Jun 29 2022, 8:41 AM
compiler-rt/lib/builtins/riscv/fp_mode.c
41–42

That's the point of csrrsi, the S stands for Set, i.e. it will OR the value you provide with the current one for you, no need to read the existing value and do the OR yourself.

Replace fsflags with csrrsi.

StephenFan marked 3 inline comments as done.Jul 5 2022, 10:59 PM
jrtc27 added inline comments.Jul 5 2022, 11:14 PM
compiler-rt/lib/builtins/riscv/fp_mode.c
19

D implies F, it's redundant

40
  1. You want csrrsi x0 (alias csrsi as the nice way to write it), not t0, otherwise you're clobbering a random register the compiler could be using (and not telling it, but in this case you don't need to clobber it in the first place).
  2. Why 0x001 rather than the readable fflags?
StephenFan added inline comments.Jul 7 2022, 7:54 PM
compiler-rt/lib/builtins/riscv/fp_mode.c
40

The spec says: "Further assembler pseudoinstructions are defined to set and clear bits in the CSR when the old value is not required: CSRS/CSRC csr, rs1; CSRSI/CSRCI csr, uimm."
So the csrsi fflags, 0x1 sets the fflags to 0x1 directly instead of 0x1 | fflags.Is my understanding correct?

jrtc27 added inline comments.Jul 7 2022, 8:59 PM
compiler-rt/lib/builtins/riscv/fp_mode.c
40

No, "the old value is not required" means that you don't want to write the old value to a register for use outside the instruction. Your "set fflags to 0x1 directly" would be csrwi fflags, 0x1 instead. csrsi fflags, 0x1 does exactly what you need here, and is merely another name for csrrsi x0, fflags.

  1. Delete checking if __riscv_d was defined since d extension imply f extension
  2. Replace csrrsi to csrsi
  3. Use fflags instead of its csr number to make code readable
StephenFan added inline comments.Jul 7 2022, 10:02 PM
compiler-rt/lib/builtins/riscv/fp_mode.c
40

Thanks for your detailed explanation!

luismarques added inline comments.Jul 29 2022, 5:21 PM
compiler-rt/lib/builtins/riscv/fp_mode.c
40

Is this actually supposed to be fflags or frm? The frm field inside fcsr starts at bit 5 so you can't just do csrsi fcsr, 1 (RISCV_INEXACT == 0x1). It would need to be csrsi fcsr, 1 << 5 (but that exceeds the 5-bit immediate, so it's not actually possible with the immediate instruction variant). I think the frm field of fcsr is also mirrored as a separate CSR, frm, so you can still use csrsi through there.

luismarques accepted this revision.Jul 29 2022, 5:47 PM

LGTM.

compiler-rt/lib/builtins/riscv/fp_mode.c
40

Nevermind. I checked the spec, NX is indeed there in fflags.

This revision is now accepted and ready to land.Jul 29 2022, 5:47 PM
This revision was landed with ongoing or failed builds.Aug 7 2022, 7:00 PM
This revision was automatically updated to reflect the committed changes.