Skip to content
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] Fix floating point #88311

Merged
merged 3 commits into from
Jul 6, 2023
Merged

[RISC-V] Fix floating point #88311

merged 3 commits into from
Jul 6, 2023

Conversation

clamp03
Copy link
Member

@clamp03 clamp03 commented Jul 3, 2023

  • Add TARGET_RISCV64 definition and fix a typo.

Below tests pass

./JIT/HardwareIntrinsics/General/Vector64_1/Vector64_1_r/Vector64_1_r.sh
./JIT/HardwareIntrinsics/General/Vector64_1/Vector64_1_ro/Vector64_1_ro.sh
./JIT/HardwareIntrinsics/General/Vector128_1/Vector128_1_r/Vector128_1_r.sh
./JIT/HardwareIntrinsics/General/Vector128_1/Vector128_1_ro/Vector128_1_ro.sh
./JIT/HardwareIntrinsics/General/Vector256_1/Vector256_1_r/Vector256_1_r.sh
./JIT/HardwareIntrinsics/General/Vector256_1/Vector256_1_ro/Vector256_1_ro.sh
./JIT/HardwareIntrinsics/General/Vector512_1/Vector512_1_r/Vector512_1_r.sh
./JIT/HardwareIntrinsics/General/Vector512_1/Vector512_1_ro/Vector512_1_ro.sh

Part of #84834
cc @wscho77 @HJLeee @JongHeonChoi @t-mustafin @alpencolt @gbalykov

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Jul 3, 2023
@clamp03 clamp03 self-assigned this Jul 3, 2023
@clamp03 clamp03 added arch-riscv Related to the RISC-V architecture and removed area-VM-coreclr community-contribution Indicates that the PR has been added by a community member labels Jul 3, 2023
@clamp03
Copy link
Member Author

clamp03 commented Jul 3, 2023

@jakobbotsch I have a question. While I fix issues in ./JIT/HardwareIntrinsics/General/, I found a problem in RISC-V. However, I cannot find a good solution by myself. Could you give any feedback?

In some tests like ./JIT/HardwareIntrinsics/General/Vector64/Vector64_r/Vector64_r.sh use Scalar<Single>.AllBitsSet which returns (T)(object)BitConverter.Int32BitsToSingle(-1). And then the test converts the Single value to Int and compare result with `-1'.

I checked converting -1 to Single (*reinterpret_cast<float*>(&i32Cns) in NI_System_BitConverter_Int32BitsToSingle) is correct in RISC-V.
Storing the value into double gtDconVal in GenTreeDblCon is a problem. It converts to canonical NaN (0x7ff8000000000000) of double not keeping original bits.

retNode = gtNewDconNode(*reinterpret_cast<float*>(&i32Cns), TYP_FLOAT);

NI_System_BitConverter_SingleToInt32Bits is too. It converts to canonical NaN 0x7fc00000 of float when copying gtDconVal to float variable. So the return value of NI_System_BitConverter_SingleToInt32Bits is not -1 anymore.
The same problem occurs in all code locations which copying float value from/to GenTreeDblCon such as assertionprops, emit, and etc.

This is a simple cpp example.

#include <stdio.h>

int main() {
    int a = -1;
    float b = *reinterpret_cast<float*>(&a);
    double c = b;
    float d = c;

    int b_i = *reinterpret_cast<int*>(&b);
    long long c_i = *reinterpret_cast<long long*>(&c);
    int d_i = *reinterpret_cast<int*>(&d);
    fprintf(stderr, "A 0x%x B 0x%x C 0x%llx D 0x%x\n", a, b_i, c_i, d_i);
    return 0;
}

RISCV: A 0xffffffff B 0xffffffff C 0x7ff8000000000000 D 0x7fc00000
The others: A 0xffffffff B 0xffffffff C 0xffffffffe0000000 D 0xffffffff (Checked on ARM32, ARM64 and AMD64)

Thank you.

@clamp03 clamp03 added area-VM-coreclr community-contribution Indicates that the PR has been added by a community member labels Jul 3, 2023
@jakobbotsch
Copy link
Member

@clamp03 @tannergooding will know more (but he's on vacation I believe, so might be a bit before he can get back to you). But my understanding is that the JIT relies on floating point conversions to preserve payload bits in NaNs, which is the behavior (I believe) for all our existing hosts.

There may be relatively few places within the JIT where such conversions happen, in which case we might be ok with a software implementation for RISC-V. However I'm not sure how the behavior extends for other operations; I don't think we would be happy about shipping a whole floating point emulation library with the JIT to align the behavior if there is a widespread difference in NaN handling.

@tannergooding
Copy link
Member

While NaN canonicalization is "technically allowed" by the IEEE 754 spec it is not recommended and there is a large amount of code that relies on NaN payloads being preserved, particularly in the presence of SIMD code where AllBitSet is used to represent a mask.

The RISC-V spec calls mention to an explicit decision to not propagate:

We considered propagating NaN payloads, as is recommended by the standard, but this decision would have increased hardware cost. Moreover, since this feature is optional in the standard, it cannot be used in portable code. Implementors are free to provide a NaN payload propagation scheme as a nonstandard extension enabled by a nonstandard operating mode. However, the canonical NaN scheme described above must always be supported and should be the default mode.

That being said, RISC-V does preserve in some cases such as FLW/FSW (floating-point load and store) and FSGNJ.S/FSGNJN.S/FSGNJX.S (floating-point sign-injection).

It is very likely that the IR generated for "bitcasting" and some other scenarios needs to take this into account and explicitly preserve the bits involved on RISC-V. This will also apply to a few, but not all, other scenarios and will likely be particularly relevant in any eventual SIMD codegen support that is added.

@tannergooding
Copy link
Member

Its also worth noting that RISC-V does define NaN boxing of Narrower Values and calls out that FMV.X.D/FMV.D.X also preserve the NaN payload.

It's possible that the codegen for holding a single-precision value in the double-precision f register is not correctly performing NaN Boxing or inserting other intermediary instructions and thus is not correctly preserving the bits.

@clamp03
Copy link
Member Author

clamp03 commented Jul 4, 2023

@jakobbotsch @tannergooding Thank you for the comments. Based on the comments, I will investigate C/C++ source in runtime and machine code generated by JITC again. Then fix those codes to use instructions which preserve the NaN payload in other PR. Could you review this PR and merge? Thank you.

@shushanhf Changed near to TARGET_LOONGARCH64, maybe you would better to review before merge it.

@shushanhf
Copy link
Contributor

@shushanhf Changed near to TARGET_LOONGARCH64, maybe you would better to review before merge it.

This PR is OK for LA64.

@clamp03 clamp03 requested review from tannergooding and jkotas July 6, 2023 02:05
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@jkotas jkotas merged commit cd9d541 into dotnet:main Jul 6, 2023
@clamp03 clamp03 deleted the fix branch July 6, 2023 05:12
@ghost ghost locked as resolved and limited conversation to collaborators Aug 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-riscv Related to the RISC-V architecture area-VM-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants