This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Combine FP_TO_INT to vfwcvt/fvncvt
ClosedPublic

Authored by luke on Jan 19 2023, 3:37 AM.

Details

Summary

Adds new pseudo instructions to make sure that the fcvt instructions
have all rounding mode (RM) and unsigned (XU) variants across
single-width, widening and narrowing conversions.
And likewise, extends the VL patterns to accompany them. We don't add
new VL nodes for the widening/narrowing conversions though, instead we
just add specific patterns for vfcvts on those wider/narrower types.

Diff Detail

Event Timeline

luke created this revision.Jan 19 2023, 3:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 19 2023, 3:37 AM
luke requested review of this revision.Jan 19 2023, 3:37 AM
luke added inline comments.Jan 19 2023, 3:43 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
11646

This is growing really unwieldy as it needs to handle the matrix of {X_F,XU_F,F_X,F_XU} x {M1,M2,M4,M8,MF2,MF4} x {vfcvt, vfwcvt, vfncvt}. Could this tablegen'ed away in another patch?

llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
5579–5582

Added in the use here to match VFCVT

5610–5613

Added in use to match VFCVT

fakepaper56 added inline comments.Jan 19 2023, 4:00 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
9592

Is it better to use VT.getScalarSizeInBits() > SrcVT.getScalarSizeInBits() * 2 || VT.getScalarSizeInBits() * 2 < SrcVT.getScalarSizeInBits() ?

luke added inline comments.Jan 19 2023, 5:09 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
9592

But wouldn't that allow invalid conversions like f32 -> i8?. From what I understand fwcvt always widens to a sew of double the size, and fncvt half

fakepaper56 added inline comments.Jan 19 2023, 5:23 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
9592

VT.getScalarSizeInBits() > SrcVT.getScalarSizeInBits() * 2 || VT.getScalarSizeInBits() * 2 < SrcVT.getScalarSizeInBits()

The expression is to deny invalid expression. f32->i8 should be avoided for the expression 8 * 2 = 16 < 32.

luke marked 2 inline comments as done.Jan 19 2023, 8:44 AM
luke added inline comments.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
9592

Ah sorry, I misunderstood. Updated

luke updated this revision to Diff 490539.Jan 19 2023, 8:44 AM
luke marked an inline comment as done.

Address comment

craig.topper added inline comments.Jan 19 2023, 2:06 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
11646

Not through any existing tablegen mechanism. This is running on MachineIR after isel. We could apply some preprocessor macros to unroll the LMUL.

llvm/lib/Target/RISCV/RISCVISelLowering.h
240

Why changing the order? This puts in a different order than the list in RISCVTargetLowering::getTargetNodeName

llvm/lib/Target/RISCV/RISCVInstrInfoVVLPatterns.td
883

I'd prefer we didn't rename things in this patch. It makes the diff bigger and makes the view in phabricator more confusing. If you want to rename, do it as a separate pre-patch.

luke added inline comments.Jan 23 2023, 10:15 AM
llvm/lib/Target/RISCV/RISCVISelLowering.h
240

I was trying to order it to match the spec, since the unsigned instructions seem to come before the signed instructions. Will take this out of this patch anyway

luke updated this revision to Diff 491447.Jan 23 2023, 10:37 AM

Remove unnecessary changes

luke marked an inline comment as done.Jan 23 2023, 10:39 AM
luke added inline comments.
llvm/lib/Target/RISCV/RISCVInstrInfoVVLPatterns.td
883

Of course, left them out for now. The intention was to bring it in line with the pseudo and spec convention of suffixing instructions that access wider register groups with w

luke marked 2 inline comments as done.Jan 23 2023, 10:40 AM
This revision is now accepted and ready to land.Jan 23 2023, 10:59 PM
This revision was landed with ongoing or failed builds.Jan 24 2023, 1:45 AM
This revision was automatically updated to reflect the committed changes.