This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Custom lowering of SET_ROUNDING
ClosedPublic

Authored by sepavloff on Nov 11 2020, 2:37 AM.

Diff Detail

Event Timeline

sepavloff created this revision.Nov 11 2020, 2:37 AM
sepavloff requested review of this revision.Nov 11 2020, 2:37 AM

Would a SELECT not be more friendly to optimisations like known-bits analysis, or does that cause too much code to be emitted in the general case?

craig.topper added inline comments.Nov 11 2020, 9:37 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
225

StdExtD implies StdExtF. So I think you only need to check StdExtF

904

Why do we need two code paths? Won't the other code constant fold if RMValue is a constant?

sepavloff updated this revision to Diff 304799.Nov 12 2020, 5:45 AM

Updated patch according to reviewer's notes.

Would a SELECT not be more friendly to optimisations like known-bits analysis, or does that cause too much code to be emitted in the general case?

Not sure if I understand you correctly, if you mean analyzing the 3 bits of rounding mode bit-by-bit, It think it does not make sense here. It is unlikely that only a part of the bits are known. Rounding mode is specified as a constant or obtained as a result of FLT_ROUNDS or as argument of a function call. Usually it is not obtained by calculations. And yes, in the general case too much code is generated, at least 8 operators, while in this implementation 5 instructions is enough.

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
225

Fixed.

904

Indeed, the general case can be optimized for constant arguments.

If this gets used in a function that does floating point, what prevents the machine schedulers from reordering the FP instructions with this? Don't we need to model FRM as a register and make the FP instructions implicitly use it? You'd also need to be using the constrained FP intrinsics which aren't implemented for RISCV yet.

lenary resigned from this revision.Jan 14 2021, 9:44 AM
sepavloff updated this revision to Diff 338760.Apr 20 2021, 1:28 AM

Repased patch

craig.topper added inline comments.Apr 20 2021, 12:53 PM
llvm/include/llvm/ADT/FloatingPointMode.h
41

This doesn't appear to be used in this patch. I don't object to adding it just wondering why it is in the RISCV patch.

llvm/test/CodeGen/RISCV/fpenv.ll
57

COMMON doesn't appear in the runline in this test.

sepavloff updated this revision to Diff 339260.Apr 21 2021, 8:49 AM

Aligned the implementation with that of FLT_ROUNDS_

sepavloff added inline comments.Apr 21 2021, 8:55 AM
llvm/test/CodeGen/RISCV/fpenv.ll
57

I changed patterns to those generated by update_llc_test_checks.py, as the test for FLT_ROUNDS_ already uses it.

This revision is now accepted and ready to land.Apr 21 2021, 9:01 AM
This revision was landed with ongoing or failed builds.Apr 22 2021, 1:05 AM
This revision was automatically updated to reflect the committed changes.