[RISC-V][JIT] Fix NaN canonicalization issue #88510

clamp03 · 2023-07-07T03:48:11Z

Based on the comments in #88311 (comment) and #88311 (comment), update codes to pass below tests.

./JIT/HardwareIntrinsics/General/Vector128/Vector128_r/Vector128_r.sh
./JIT/HardwareIntrinsics/General/Vector256/Vector256_r/Vector256_r.sh
./JIT/HardwareIntrinsics/General/Vector512/Vector512_r/Vector512_r.sh
./JIT/HardwareIntrinsics/General/Vector256/Vector256_ro/Vector256_ro.sh
./JIT/HardwareIntrinsics/General/Vector128/Vector128_ro/Vector128_ro.sh
./JIT/HardwareIntrinsics/General/Vector512/Vector512_ro/Vector512_ro.sh
./JIT/HardwareIntrinsics/General/Vector64/Vector64_r/Vector64_r.sh
./JIT/HardwareIntrinsics/General/Vector64/Vector64_ro/Vector64_ro.sh

I fixed all codes what I found. Minimum changes to pass the tests are those in emit.cpp, valuenum.cpp and NI_System_BitConverter_Int32BitsToSingle and NI_System_BitConverter_SingleToInt32Bits in importercalls.cpp.

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

ghost · 2023-07-07T03:48:29Z

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

Based on the comments in #88311 (comment) and #88311 (comment), update codes to pass below tests.

./JIT/HardwareIntrinsics/General/Vector128/Vector128_r/Vector128_r.sh
./JIT/HardwareIntrinsics/General/Vector256/Vector256_r/Vector256_r.sh
./JIT/HardwareIntrinsics/General/Vector512/Vector512_r/Vector512_r.sh
./JIT/HardwareIntrinsics/General/Vector256/Vector256_ro/Vector256_ro.sh
./JIT/HardwareIntrinsics/General/Vector128/Vector128_ro/Vector128_ro.sh
./JIT/HardwareIntrinsics/General/Vector512/Vector512_ro/Vector512_ro.sh
./JIT/HardwareIntrinsics/General/Vector64/Vector64_r/Vector64_r.sh
./JIT/HardwareIntrinsics/General/Vector64/Vector64_ro/Vector64_ro.sh

I fixed all codes what I found. Minimum changes to pass the tests are those in emit.cpp, valuenum.cpp and NI_System_BitConverter_Int32BitsToSingle and NI_System_BitConverter_SingleToInt32Bits in importercalls.cpp.

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

Author:	clamp03
Assignees:	clamp03
Labels:	`area-CodeGen-coreclr`, `community-contribution`
Milestone:	-

jakobbotsch

This logic should be extracted into a new helper in FloatingPointUtils, e.g. FloatingPointUtils::ConvertToDouble that preserves the payload bits and is a noop for everything but RISCV64. We do not want to add these #ifdef TARGET_RISCV everywhere.

jakobbotsch · 2023-07-07T06:45:05Z

src/coreclr/jit/assertionprop.cpp

+                                goto DONE_ASSERTION;
+                            }
+                        }
+#endif // TARGET_RISCV64


Why is this change necessary? _isnan should not care about payload bits.

Sure. I will make a new helper. Thank you. I wanted to know proper location and name. :)

Because when a float value is converted to Double with preserving payload and saved in DconValue, the saved Double value is not 'NaN' in Double type.
For example, c can be assumed as f64Cns in the change.

int a = -1; double c = *reinterpret_cast<double*>(&a); float d = *reinterpret_cast<float*>(&c); float e = (float)c; fprintf(stderr, "%lf %f %f %d %d %d\n", c, d, e, isnan(c), isnan(d), isnan(e));

0.000000 nan 0.000000 0 1 0
Only the *reinterpret_cast<float*>(&c) is NaN in the example.
If I misunderstand your comment, please let me know.
Thank you!

Conversion to a double by reinterpreting a 32 bit int is not correct. The right way to do it is something that constructs the uint64_t bits directly by ORing in the desired payload bits in the right place and returns UInt64BitsToDouble from that. It should mimic what our other hosts do for float -> double conversions. We should not be trying to store a 32 bit float in the 64 bit double field.

I currently do not have access to a PC, but @tannergooding may be able to help with the exact right way to do the conversion that retains the payload bits on the proper way.

The PR updates codes to use fsd/flw (preserving payload) or fcvt (not preserving payload) in case of float is NaN or not.
I will wait for the exact solution. Thank you so much!!!

I think there are a few possible options. For the JIT side, all of them have the issue that the general codebase would need to be "audited" to ensure that all float->double handling is done correctly. We're not consistent about this today.

Update GenTreeDblCon to be more like GenTreeIcon

You can update GenTreeDblCon to have a union of gtDconVal and gtFconVal. It would then expose float FconValue() and void SetFconValue(float value) and assert that Dcon is only called for TYP_DOUBLE and Fcon is only called for `TYP_FLOAT

This will ensure that the values aren't ever accessed incorrectly and avoids the upcast/downcast issue entirely for the JIT. It does not resolve the issue for managed code and will likely have some minor TP hit due to needing to add branching to various callsites in the JIT.

Expose a payload preserving conversion helper

Just as it says, define a helper API that does a payload preserving conversion. This would be a software based conversion and would simply ensure the significand bits are copied up and truncated down, just as happens for other platforms.

This helper would be used by both the JIT and managed and ensures managed code also gets the right handling. This would incur a branch check per conversion on RISC-V.

Explicitly utilize FMV.n.X/FMV.X.n

These instructions allow storing n-bits in the larger f register, preserving the payload. The value stored is a NaN boxed value.

One could utilize this to more efficiently store the data in gtDconVal, but it has many of the same issues/considerations where it needs to be done everywhere and consistently; otherwise subtle bugs could creep in.

I'd suggest to introduce the payload preserving conversion helper for now and use it in the cases found in this PR. I think that's going to be the least invasive fix. We can consider the more complete/invasive fixes as part of .NET 9.

@jakobbotsch @tannergooding Thank you so much for the comments.
I made a conversion helper and updated all. However, I don't know what is the right way to convert, so I just retained the conversion method as it is. Could you please give the right way what you mentioned?

Conversion to a double by reinterpreting a 32 bit int is not correct. The right way to do it is something that constructs the uint64_t bits directly by ORing in the desired payload bits in the right place and returns UInt64BitsToDouble from that.

@jakobbotsch @tannergooding Thank you so much for the comments. I made a conversion helper and updated all. However, I don't know what is the right way to convert, so I just retained the conversion method as it is. Could you please give the right way what you mentioned?

Conversion to a double by reinterpreting a 32 bit int is not correct. The right way to do it is something that constructs the uint64_t bits directly by ORing in the desired payload bits in the right place and returns UInt64BitsToDouble from that.

Could you take a look? Thank you so much.

@clamp03 The conversion functions should look something like:

//------------------------------------------------------------------------ // convertToDouble: Convert a single to a double with platform independent // preservation of payload bits. // // Arguments: // f - the single // // Return Value: // A double. // // Remarks: // All our host platforms except for RISCV-64 will preserve payload bits of // NaNs. This function implements the conversion in software for RISCV-64 to // mimic other platforms. // double FloatingPointUtils::convertToDouble(float f) { #ifdef HOST_RISCV64 if (f == f) { return f; } uint32_t bits = BitOperations::SingleToUInt32Bits(f); uint32_t payload = bits & ((1u << 23) - 1); uint64_t newBits = ((uint64_t)(bits >> 31) << 63) | 0x7FF8'0000'0000'0000ul | ((uint64_t)payload << 29); return BitOperations::UInt64BitsToDouble(newBits); #else return f; #endif } //------------------------------------------------------------------------ // convertToSingle: Convert a double to a single with platform independent // preservation of payload bits. // // Arguments: // d - the double // // Return Value: // A float. // // Remarks: // All our host platforms except for RISCV-64 will preserve payload bits of // NaNs. This function implements the conversion in software for RISCV-64 to // mimic other platforms. // float FloatingPointUtils::convertToSingle(double d) { #ifdef HOST_RISCV64 if (d == d) { return (float)d; } uint64_t bits = BitOperations::DoubleToUInt64Bits(d); uint32_t newPayload = (uint32_t)((bits >> 29) & ((1u << 23) - 1)); uint32_t newBits = ((uint32_t)(bits >> 63) << 31) | 0x7F80'0000u | newPayload; return BitOperations::UInt32BitsToSingle(newBits); #else return (float)d; #endif }

You should change the JIT to use this conversion function in the places you have identified.

Awesome!!! I didn't think so. Thank you so much.

jakobbotsch · 2023-07-20T09:13:02Z

src/coreclr/jit/importercalls.cpp

@@ -3794,7 +3794,8 @@ GenTree* Compiler::impIntrinsic(GenTree*                newobjThis,
                if (op1->IsIntegralConst())
                {
                    int32_t i32Cns = (int32_t)op1->AsIntConCommon()->IconValue();


Suggested change

int32_t i32Cns = (int32_t)op1->AsIntConCommon()->IconValue();

jakobbotsch · 2023-07-20T09:13:08Z

src/coreclr/jit/importercalls.cpp

@@ -3794,7 +3794,8 @@ GenTree* Compiler::impIntrinsic(GenTree*                newobjThis,
                if (op1->IsIntegralConst())
                {
                    int32_t i32Cns = (int32_t)op1->AsIntConCommon()->IconValue();
-                    retNode        = gtNewDconNode(*reinterpret_cast<float*>(&i32Cns), TYP_FLOAT);
+                    float   f32Cns = *reinterpret_cast<float*>(&i32Cns);


Suggested change

float f32Cns = *reinterpret_cast<float*>(&i32Cns);

float f32Cns = BitOperations::UInt32BitsToSingle((uint32_t)op1->AsIntConCommon()->IconValue());

Now that you're here...

jakobbotsch · 2023-07-20T09:14:10Z

src/coreclr/jit/importercalls.cpp

@@ -3834,7 +3835,7 @@ GenTree* Compiler::impIntrinsic(GenTree*                newobjThis,

                if (op1->IsCnsFltOrDbl())
                {
-                    float f32Cns = (float)op1->AsDblCon()->DconValue();
+                    float f32Cns = FloatingPointUtils::convertToSingle(op1->AsDblCon()->DconValue());
                    retNode      = gtNewIconNode(*reinterpret_cast<int32_t*>(&f32Cns));


Suggested change

retNode = gtNewIconNode(*reinterpret_cast<int32_t*>(&f32Cns));

retNode = gtNewIconNode((int32_t)BitOperations::SingleToUInt32Bits(f32Cns));

(since we're here anyways)

jakobbotsch · 2023-07-20T09:15:52Z

src/coreclr/jit/assertionprop.cpp

+                        conValTree =
+                            gtNewDconNode(FloatingPointUtils::convertToDouble(*reinterpret_cast<float*>(&value)),
+                                          TYP_FLOAT);


Suggested change

conValTree =

gtNewDconNode(FloatingPointUtils::convertToDouble(*reinterpret_cast<float*>(&value)),

TYP_FLOAT);

conValTree =

gtNewDconNode(FloatingPointUtils::convertToDouble(BitOperations::UInt32BitsToSingle((uint32_t)value)),

TYP_FLOAT);

clamp03 · 2023-07-20T09:45:47Z

Thank you so much. I will think and try more for the better codes.

jakobbotsch

Thanks!

[RISC-V][JIT] Fix canonical NaN issue

0d5d5a9

clamp03 self-assigned this Jul 7, 2023

ghost added the community-contribution Indicates that the PR has been added by a community member label Jul 7, 2023

dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jul 7, 2023

clamp03 added the arch-riscv Related to the RISC-V architecture label Jul 7, 2023

jakobbotsch requested changes Jul 7, 2023

View reviewed changes

ghost added needs-author-action An issue or pull request that requires more info or actions from the author. and removed needs-author-action An issue or pull request that requires more info or actions from the author. labels Jul 7, 2023

[RISC-V][JIT] Update to use helpers

3904786

build-analysis bot mentioned this pull request Jul 10, 2023

simpleruntimeeventvalidation test failing in CI #88499

Closed

runfoapp bot mentioned this pull request Jul 10, 2023

Infra improvements for Helix #68176

Closed

clamp03 closed this Jul 18, 2023

clamp03 reopened this Jul 18, 2023

This was referenced Jul 18, 2023

Tracking issue for CI build timeouts #76454

Closed

System.Tests.PosixSignalRegistrationTests.SignalCanCancelTermination is occasionally failing on macOS #63760

Closed

[RISC-V][JIT] Update conversion functions

d267584

jakobbotsch reviewed Jul 20, 2023

View reviewed changes

[RISC-V][JIT] Update

e46699a

gbalykov mentioned this pull request Jul 21, 2023

CoreCLR RISC-V architecture port #84834

Open

7 tasks

jakobbotsch approved these changes Jul 21, 2023

View reviewed changes

jakobbotsch merged commit 84bffc8 into dotnet:main Jul 21, 2023

clamp03 deleted the fixNaN branch July 24, 2023 02:39

ghost locked as resolved and limited conversation to collaborators Aug 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RISC-V][JIT] Fix NaN canonicalization issue #88510

[RISC-V][JIT] Fix NaN canonicalization issue #88510

clamp03 commented Jul 7, 2023

ghost commented Jul 7, 2023

jakobbotsch left a comment •

edited

Loading

jakobbotsch Jul 7, 2023

clamp03 Jul 7, 2023 •

edited

Loading

jakobbotsch Jul 7, 2023

clamp03 Jul 7, 2023 •

edited

Loading

tannergooding Jul 7, 2023

jakobbotsch Jul 7, 2023

clamp03 Jul 10, 2023

clamp03 Jul 14, 2023

jakobbotsch Jul 19, 2023

clamp03 Jul 20, 2023

jakobbotsch Jul 20, 2023

jakobbotsch Jul 20, 2023

jakobbotsch Jul 20, 2023 •

edited

Loading

jakobbotsch Jul 20, 2023

clamp03 commented Jul 20, 2023

jakobbotsch left a comment

	float f32Cns = reinterpret_cast<float>(&i32Cns);
	float f32Cns = BitOperations::UInt32BitsToSingle((uint32_t)op1->AsIntConCommon()->IconValue());

	retNode = gtNewIconNode(reinterpret_cast<int32_t>(&f32Cns));
	retNode = gtNewIconNode((int32_t)BitOperations::SingleToUInt32Bits(f32Cns));

[RISC-V][JIT] Fix NaN canonicalization issue #88510

[RISC-V][JIT] Fix NaN canonicalization issue #88510

Conversation

clamp03 commented Jul 7, 2023

ghost commented Jul 7, 2023

jakobbotsch left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clamp03 Jul 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clamp03 Jul 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Update GenTreeDblCon to be more like GenTreeIcon

Expose a payload preserving conversion helper

Explicitly utilize FMV.n.X/FMV.X.n

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakobbotsch Jul 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clamp03 commented Jul 20, 2023

jakobbotsch left a comment

Choose a reason for hiding this comment

jakobbotsch left a comment •

edited

Loading

clamp03 Jul 7, 2023 •

edited

Loading

clamp03 Jul 7, 2023 •

edited

Loading

Explicitly utilize `FMV.n.X`/`FMV.X.n`

jakobbotsch Jul 20, 2023 •

edited

Loading