Page MenuHomePhabricator

[RISCV] Add support for RVV int<->fp & fp<->fp conversions
ClosedPublic

Authored by frasercrmck on Tue, Jan 26, 7:51 AM.

Details

Summary

This patch adds support for the full range of vector int-to-float,
float-to-int, and float-to-float conversions on legal types.

Many conversions are supported natively in RVV so are lowered with
patterns. These include conversions between (element) types of the same
size, and those that are half/double the size of the input. When
conversions take place between types that are less than half or more
than double the size we must lower them using sequences of instructions
which go via intermediate types.

Diff Detail

Event Timeline

frasercrmck created this revision.Tue, Jan 26, 7:51 AM
frasercrmck requested review of this revision.Tue, Jan 26, 7:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Jan 26, 7:51 AM
jrtc27 added inline comments.Tue, Jan 26, 7:53 AM
llvm/test/CodeGen/RISCV/rvv/vfpext-sdnode-rv64.ll
3 ↗(On Diff #319296)

These tests look very similar to the rv64 ones; please combine files whenever possible.

frasercrmck marked an inline comment as done.Tue, Jan 26, 8:08 AM
frasercrmck added inline comments.
llvm/test/CodeGen/RISCV/rvv/vfpext-sdnode-rv64.ll
3 ↗(On Diff #319296)

Yeah, that's a good idea, cheers. I'll also update my scripts to prevent this in the future.

frasercrmck marked an inline comment as done.
  • merge rv32/rv64 tests
craig.topper added inline comments.Tue, Jan 26, 12:36 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
803

I'm a bit surprised this works. Back to back FP_EXTEND seems like an obvious DAG combine.

808

I think it's in the spec as round-to-odd

836

getSizeInBits returns an unsigned.

838

Once the uint64_ts above are unsigned this can be isPowerOf2_32.

866

Anything large enough to trigger rounding on the float conversion, is too large to fit in f16 anyway isn't it? So it will become infinity I think.

frasercrmck marked 2 inline comments as done.
  • rebase on main
  • change comment: round-towards-odd -> round-to-odd
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
803

Yeah, I'm surprised too. I was toying with the idea of introducing a custom node to avoid the possibility of an infinite loop, but didn't know whether that's the kind of thing to do upfront without a test case. What do you think?

I think for some of the others we might be okay as I don't think it's legal to combine back-to-back FP_ROUND.

808

Ah yeah, it is, thanks. I was going from the description of vfncvt.rod (0.91) which says "convert double-width float to single-width float, rounding towards odd". But I think round-to-odd is better as it's describing the rounding mode itself.

836

Correct me if I'm wrong, but since it returns TypeSize, it returns uint64_t as its underlying ScalarTy as per:

// This class is used to represent the size of types. If the type is of fixed
class TypeSize;
template <> struct LinearPolyBaseTypeTraits<TypeSize> {
  using ScalarTy = uint64_t;
  static constexpr unsigned Dimensions = 2;
};

(Interestingly the comment trails off there. Might need to fix that up)

866

Yeah that's what I came round to thinking. I'll remove the TODO before it's merged.

craig.topper added inline comments.Wed, Jan 27, 9:43 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
803

Looks like at least ARM also relies on this not being combined. So they will break too if someone adds the DAG combine. So I think it's fine to do the same.

836

It used to return unsigned before TypeSize was introduced. I guess I'm used to seeing unsigned everywhere. Sorry for the noise.

frasercrmck marked 5 inline comments as done.Wed, Jan 27, 1:18 PM
frasercrmck added inline comments.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
803

Thanks for looking into that.

836

No worries. Having a wee look, I think there are a few places we explicitly use unsigned which we can clear up for posterity.

craig.topper added inline comments.Wed, Jan 27, 1:24 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
836

unsigned is of course more efficient when the compiler is built to run on a 32 bit CPU. And vectors and especially their elements are never going to need 64 bits to represent.

frasercrmck marked 2 inline comments as done.
  • rebase on main
  • use unsigned for size-in-bits
This revision is now accepted and ready to land.Thu, Jan 28, 1:50 AM
This revision was landed with ongoing or failed builds.Thu, Jan 28, 1:57 AM
This revision was automatically updated to reflect the committed changes.