-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[RISC-V] Fused multiply-add #113961
base: main
Are you sure you want to change the base?
[RISC-V] Fused multiply-add #113961
Conversation
target_compile_definitions(${TARGETDETAILS_TARGET} PRIVATE FEATURE_SIMD) | ||
target_compile_definitions(${TARGETDETAILS_TARGET} PRIVATE FEATURE_HW_INTRINSICS) | ||
target_compile_definitions(${TARGETDETAILS_TARGET} PRIVATE FEATURE_MASKED_HW_INTRINSICS) | ||
elseif (TARGETDETAILS_ARCH STREQUAL "riscv64") | ||
target_compile_definitions(${TARGETDETAILS_TARGET} PRIVATE FEATURE_HW_INTRINSICS) | ||
endif () |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still hesitating whether separating FEATURE_HW_INTRINSICS from SIMD and MASKED_HW_INTRINSICS is the right approach, there's been little code separation between them. Until we bring up the "V" extension the intrinsics RISC-V needs are scalar (e.g. FusedMultiplyAdd, LeadingZeroCount, PopCount, MultiplyHigh) and usually covered in common CoreLib parts like System.Math.
An alternative would be to handle them like #113689 and fix the value numbering for 3-operand GT_INTRINSIC to get FMA:
runtime/src/coreclr/jit/importercalls.cpp
Lines 4312 to 4316 in b21c93c
// TODO-CQ-XArch: Ideally we would create a GT_INTRINSIC node for fma, however, that currently | |
// requires more extensive changes to valuenum to support methods with 3 operands | |
// We want to generate a GT_INTRINSIC node in the case the call can't be treated as | |
// a target intrinsic so that we can still benefit from CSE and constant folding. |
Not sure if changes would be less extensive than maintaining the above mentioned separation.
@dotnet/jit-contrib @jakobbotsch if you have an opinion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LeadingZeroCount and friends look doable via GT_INTRINSIC (WiP). So that leaves us with FMA as the odd one out.
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
RISC-V Release-CLR-VF2: 9527 / 9547 (99.79%)
Release-CLR-VF2.md, Release-CLR-VF2.xml, testclr_output.tar.gz Build information and commandsGIT: RISC-V Release-FX-VF2: 423645 / 458893 (92.32%)
Build information and commandsGIT: RISC-V Release-CLR-QEMU: 9527 / 9547 (99.79%)
Release-CLR-QEMU.md, Release-CLR-QEMU.xml, testclr_output.tar.gz Build information and commandsGIT: RISC-V Release-FX-QEMU: 632088 / 656489 (96.28%)
Release-FX-QEMU.md, Release-FX-QEMU.xml, testfx_output.tar.gz Build information and commandsGIT: |
@@ -34,6 +34,19 @@ static const HWIntrinsicInfo hwIntrinsicInfoArray[] = { | |||
/* category */ category \ | |||
}, | |||
#include "hwintrinsiclistarm64.h" | |||
#elif defined (TARGET_RISCV64) | |||
#define HARDWARE_INTRINSIC(isa, name, size, numarg, t1, t2, t3, t4, t5, t6, t7, t8, t9, t10, category, flag) \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: maybe better to define this macro once under #if defined(TARGET_XARCH) || defined (TARGET_ARM64) || defined (TARGET_RISCV64)
.
Part of #84834, cc @dotnet/samsung