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.
Details
- Reviewers
asb craig.topper
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Doesn't this render fesetround useless?
llvm/test/CodeGen/RISCV/double-arith.ll | ||
---|---|---|
37 | 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. |
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 | ||
---|---|---|
37 | I see. Changed assertions in the produced assembler. |
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 | 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. |
llvm/lib/Target/RISCV/RISCVInstrInfoF.td | ||
---|---|---|
402 | The former was correct. |
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.
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.