This is an archive of the discontinued LLVM Phabricator instance.

[RISCV][CodeGen] Add codegen pattern for experimental zfa extension (FLI and FCVTMOD not included)
ClosedPublic

Authored by joshua-arch1 on Feb 13 2023, 11:10 PM.

Details

Summary

This patch implements experimental support for the RISCV Zfa extension except FLI and FCVTMOD instructions as specified here: https://github.com/riscv/riscv-isa-manual/releases/download/draft-20221119-5234c63/riscv-spec.pdf, Ch. 25. This extension has not been ratified. Once ratified, it'll move out of experimental status.

This change adds codegen support.

Diff Detail

Event Timeline

joshua-arch1 created this revision.Feb 13 2023, 11:10 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 13 2023, 11:10 PM
joshua-arch1 requested review of this revision.Feb 13 2023, 11:10 PM

You don’t need to mention FCVTMOD given it does not apply to C/C++.

joshua-arch1 added a comment.EditedFeb 14 2023, 2:48 AM

You don’t need to mention FCVTMOD given it does not apply to C/C++.

Maybe we can define intrinsics like llvm.fptosi.sat.i32.f64 but without saturating. If feasible, I can have a try.

craig.topper accepted this revision.Feb 14 2023, 9:43 AM

LGTM with that one formatting fix.

llvm/lib/Target/RISCV/RISCVInstrInfoZfa.td
91

This } should line up with the l in let

This revision is now accepted and ready to land.Feb 14 2023, 9:43 AM
asb added a comment.Feb 14 2023, 9:52 AM

I think function renaming was missed in half-zfa.ll.

Also, most of these tests have the same output for RV32 and RV64, so would benefit from having common CHECK prefixes for them both (e.g. CHECKIZFA), so visual inspection is easier.

llvm/test/CodeGen/RISCV/half-zfa.ll
10

The tests in this file should presumable be e.g. fminm_h rather than e.g. fminm_d (seems to be a repeated mistake elsewhere in this file).

You don’t need to mention FCVTMOD given it does not apply to C/C++.

This was not fixed in the committed version, nor was the Differential Revision: link included, nor reviewer information. Please use arc patch/amend appropriately in future, it's annoying when commits don't have this information as it's a hassle to have to go back and find the right revision manually.

You don’t need to mention FCVTMOD given it does not apply to C/C++.

This was not fixed in the committed version, nor was the Differential Revision: link included, nor reviewer information. Please use arc patch/amend appropriately in future, it's annoying when commits don't have this information as it's a hassle to have to go back and find the right revision manually.

I see. I'll pay attention next time.

asb added a comment.Feb 15 2023, 11:46 PM

I think function renaming was missed in half-zfa.ll.

It looks like this was addressed in the committed version - thanks!

Also, most of these tests have the same output for RV32 and RV64, so would benefit from having common CHECK prefixes for them both (e.g. CHECKIZFA), so visual inspection is easier.

I think you might have missed my comment here.

This revision was landed with ongoing or failed builds.Feb 17 2023, 10:28 AM
This revision was automatically updated to reflect the committed changes.