This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Fix rounding mode in lowering of float operations
AbandonedPublic

Authored by sepavloff on Dec 21 2020, 3:17 AM.

Details

Reviewers
asb
craig.topper
Summary

LLVM IR nodes like 'fadd', 'fmul' and other are assumed to execute in the
default floating-point environment, which in turn assumes rounding to
nearest even. In RISCV these operations were mapped to dynamic rounding
variants. This change fixes this by mapping to instructions with RNE
argument.

Diff Detail

Event Timeline

sepavloff created this revision.Dec 21 2020, 3:17 AM
sepavloff requested review of this revision.Dec 21 2020, 3:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 21 2020, 3:17 AM
Herald added a subscriber: MaskRay. · View Herald Transcript

Doesn't this render fesetround useless?

llvm/test/CodeGen/RISCV/double-arith.ll
40

I don't understand this. Either you're changing CodeGen which should mean the assembly changes or you're changing the assembler which should instead be tested in the MC tests. You should almost never need to disassemble during a CodeGen test.

sepavloff updated this revision to Diff 313097.Dec 21 2020, 6:25 AM

Changed test

Doesn't this render fesetround useless?

No. fesetround sets dynamic rounding mode (register frm), which may be any mode. However in the case of non-default rounding mode (all other than RNE), floating operations must be represented by constrained intrinsics, for example llvm.experimental.constrained.fadd should be used instead of fadd.

llvm/test/CodeGen/RISCV/double-arith.ll
40

I see. Changed assertions in the produced assembler.

Doesn't this render fesetround useless?

No. fesetround sets dynamic rounding mode (register frm), which may be any mode. However in the case of non-default rounding mode (all other than RNE), floating operations must be represented by constrained intrinsics, for example llvm.experimental.constrained.fadd should be used instead of fadd.

Ah, I see, I need -ffp-strict (or an FENV_ACCESS pragma) if I want that behaviour. Though is that something people actually use? I have a feeling that a lot of code out there uses fesetround without changing compiler flags, and probably "works" on x86 because everything uses the dynamic rounding mode. Can we not just always use the dynamic rounding mode to be more friendly to users, and say that if you change the rounding mode you should have told the compiler about it (but you'll get behaviour approximately like what you asked for)? What do other backends do?

FWIW, the Clang documentation just says:

The option -fno-rounding-math allows the compiler to assume that the rounding mode is set to FE_TONEAREST. This is the default.

So this revision should only make a difference when people violate the documented assumptions of their compiler.

I don't see a strong reason to change this. Using dynamic is consistent with targets that only have a dynamic mode.

But if we were to change this, shouldn't also update the Zfh extension files half-arith.ll and RISCVInstrInfoZfh.td.

llvm/lib/Target/RISCV/RISCVInstrInfoF.td
402–406

This comment previously said gcc was using dynamic rounding mode. Now it says gcc uses nearest even. Only one of those statements can be true.

jrtc27 added inline comments.Dec 21 2020, 9:46 AM
llvm/lib/Target/RISCV/RISCVInstrInfoF.td
402–406

The former was correct.

I have a feeling that a lot of code out there uses fesetround without changing compiler flags, and probably "works" on x86 because everything uses the dynamic rounding mode.

I think you are right. Using dynamic rounding mode, the fact that fesetround is an external function (thus may have side effect which restricts code reordering) and special care taken by compilers to limit optimizations of FP operations allow the generated code to work in many cases. Another possible reason of "working" may be related to previous unavailability of #pragma STDC FENV_ACCESS. People that used fesetround understand that they should use this pragma but it was unsupported, so they try to use workarounds instead of complaining.

Can we not just always use the dynamic rounding mode to be more friendly to users, and say that if you change the rounding mode you should have told the compiler about it (but you'll get behaviour approximately like what you asked for)? What do other backends do?

If only rounding mode was an issue, probably yes. There is however dependency on another FP register, fflags. More detailed consideration is make in D94163.

FWIW, the Clang documentation just says:

The option -fno-rounding-math allows the compiler to assume that the rounding mode is set to FE_TONEAREST. This is the default.

So this revision should only make a difference when people violate the documented assumptions of their compiler.

If only the default mode were needed, this change were useless. This is a step toward support of non-default FP environments.

sepavloff abandoned this revision.Jan 6 2021, 2:36 AM

This change is merged into D94163 and subsequent changes.