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.

Sign up for GitHub

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][JIT] Fix NaN canonicalization issue #88510

Merged
merged 4 commits into from
Jul 21, 2023
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0d5d5a9
[RISC-V][JIT] Fix canonical NaN issue
clamp03 Jul 6, 2023
3904786
[RISC-V][JIT] Update to use helpers
clamp03 Jul 7, 2023
d267584
[RISC-V][JIT] Update conversion functions
clamp03 Jul 20, 2023
e46699a
[RISC-V][JIT] Update
clamp03 Jul 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 32 additions & 4 deletions src/coreclr/jit/assertionprop.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1200,8 +1200,18 @@ AssertionIndex Compiler::optCreateAssertion(GenTree* op1,
}
else
{
noway_assert(op2->gtOper == GT_CNS_DBL);
/* If we have an NaN value then don't record it */
noway_assert(op2->gtOper == GT_CNS_DBL);
#ifdef TARGET_RISCV64
if (op2->TypeGet() == TYP_FLOAT)
{
double f64Cns = op2->AsDblCon()->DconValue();
if (_isnan(*reinterpret_cast<float*>(&f64Cns)))
{
goto DONE_ASSERTION;
}
}
#endif // TARGET_RISCV64
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@clamp03 clamp03 Jul 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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!

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@clamp03 clamp03 Jul 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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!!!

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

@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.

Copy link
Member Author

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

@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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

if (_isnan(op2->AsDblCon()->DconValue()))
{
goto DONE_ASSERTION; // Don't make an assertion
Expand Down Expand Up @@ -2587,7 +2597,16 @@ GenTree* Compiler::optVNConstantPropOnTree(BasicBlock* block, GenTree* tree)
{
// Implicit conversion to float or double
assert(varTypeIsFloating(tree->TypeGet()));
conValTree = gtNewDconNode(value, tree->TypeGet());
#ifdef TARGET_RISCV64
if (tree->TypeGet() == TYP_FLOAT && _isnan(*reinterpret_cast<float*>(&value)))
{
conValTree = gtNewDconNode(*reinterpret_cast<double*>(&value), tree->TypeGet());
}
else
#endif // TARGET_RISCV64
{
conValTree = gtNewDconNode(value, tree->TypeGet());
}
}
break;
}
Expand Down Expand Up @@ -2696,8 +2715,17 @@ GenTree* Compiler::optVNConstantPropOnTree(BasicBlock* block, GenTree* tree)
break;

case TYP_FLOAT:
// Same sized reinterpretation of bits to float
conValTree = gtNewDconNode(*reinterpret_cast<float*>(&value), TYP_FLOAT);
#ifdef TARGET_RISCV64
if (_isnan(*reinterpret_cast<float*>(&value)))
{
conValTree = gtNewDconNode(*reinterpret_cast<double*>(&value), TYP_FLOAT);
}
else
#endif // TARGET_RISCV64
{
// Same sized reinterpretation of bits to float
conValTree = gtNewDconNode(*reinterpret_cast<float*>(&value), TYP_FLOAT);
}
break;

case TYP_DOUBLE:
Expand Down
10 changes: 9 additions & 1 deletion src/coreclr/jit/emit.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8050,7 +8050,15 @@ CORINFO_FIELD_HANDLE emitter::emitFltOrDblConst(double constValue, emitAttr attr

if (attr == EA_4BYTE)
{
f = forceCastToFloat(constValue);
#ifdef TARGET_RISCV64
f = *reinterpret_cast<float*>(&constValue);
if (!FloatingPointUtils::isNaN(f))
{
f = forceCastToFloat(constValue);
}
#else
f = forceCastToFloat(constValue);
#endif // TARGET_RISCV64
cnsAddr = &f;
dataType = TYP_FLOAT;
}
Expand Down
50 changes: 44 additions & 6 deletions src/coreclr/jit/importercalls.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1556,7 +1556,7 @@ GenTree* Compiler::impFixupCallStructReturn(GenTreeCall* call, CORINFO_CLASS_HAN
const ReturnTypeDesc* retTypeDesc = call->GetReturnTypeDesc();
const unsigned retRegCount = retTypeDesc->GetReturnRegCount();
#else // !FEATURE_MULTIREG_RET
const unsigned retRegCount = 1;
const unsigned retRegCount = 1;
#endif // !FEATURE_MULTIREG_RET

structPassingKind howToReturnStruct;
Expand Down Expand Up @@ -3794,7 +3794,17 @@ GenTree* Compiler::impIntrinsic(GenTree* newobjThis,
if (op1->IsIntegralConst())
{
int32_t i32Cns = (int32_t)op1->AsIntConCommon()->IconValue();
Copy link
Member

Choose a reason for hiding this comment

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

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

retNode = gtNewDconNode(*reinterpret_cast<float*>(&i32Cns), TYP_FLOAT);
#ifdef TARGET_RISCV64
float f32Cns = *reinterpret_cast<float*>(&i32Cns);
if (FloatingPointUtils::isNaN(f32Cns))
{
retNode = gtNewDconNode(*reinterpret_cast<double*>(&i32Cns), TYP_FLOAT);
}
else
#endif // TARGET_RISCV64
{
retNode = gtNewDconNode(*reinterpret_cast<float*>(&i32Cns), TYP_FLOAT);
}
}
else
{
Expand Down Expand Up @@ -3834,8 +3844,17 @@ GenTree* Compiler::impIntrinsic(GenTree* newobjThis,

if (op1->IsCnsFltOrDbl())
{
float f32Cns = (float)op1->AsDblCon()->DconValue();
retNode = gtNewIconNode(*reinterpret_cast<int32_t*>(&f32Cns));
#ifdef TARGET_RISCV64
double f64Cns = op1->AsDblCon()->DconValue();
float f32Cns = *reinterpret_cast<float*>(&f64Cns);
if (!FloatingPointUtils::isNaN(f32Cns))
{
f32Cns = (float)op1->AsDblCon()->DconValue();
}
#else
float f32Cns = (float)op1->AsDblCon()->DconValue();
#endif // TARGET_RISCV64
retNode = gtNewIconNode(*reinterpret_cast<int32_t*>(&f32Cns));
}
else
{
Expand Down Expand Up @@ -4141,7 +4160,16 @@ GenTree* Compiler::impSRCSUnsafeIntrinsic(NamedIntrinsic intrinsic,
else
{
assert(fromType == TYP_FLOAT);
float f32Cns = static_cast<float>(op1->AsDblCon()->DconValue());
#ifdef TARGET_RISCV64
double f64Cns = op1->AsDblCon()->DconValue();
float f32Cns = *reinterpret_cast<float*>(&f64Cns);
if (!FloatingPointUtils::isNaN(f32Cns))
{
f32Cns = static_cast<float>(op1->AsDblCon()->DconValue());
}
#else
float f32Cns = static_cast<float>(op1->AsDblCon()->DconValue());
#endif // TARGET_RISCV64
return gtNewIconNode(static_cast<int32_t>(BitOperations::SingleToUInt32Bits(f32Cns)));
}
}
Expand Down Expand Up @@ -4181,7 +4209,17 @@ GenTree* Compiler::impSRCSUnsafeIntrinsic(NamedIntrinsic intrinsic,
assert(toType == TYP_FLOAT);

uint32_t u32Cns = static_cast<uint32_t>(op1->AsIntConCommon()->IconValue());
return gtNewDconNode(BitOperations::UInt32BitsToSingle(u32Cns), TYP_FLOAT);
#ifdef TARGET_RISCV64
float f32Cns = BitOperations::UInt32BitsToSingle(u32Cns);
if (FloatingPointUtils::isNaN(f32Cns))
{
return gtNewDconNode(*reinterpret_cast<double*>(&f32Cns), TYP_FLOAT);
}
else
#endif // TARGET_RISCV64
{
return gtNewDconNode(BitOperations::UInt32BitsToSingle(u32Cns), TYP_FLOAT);
}
}
}
else
Expand Down
12 changes: 11 additions & 1 deletion src/coreclr/jit/valuenum.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10345,7 +10345,17 @@ void Compiler::fgValueNumberTreeConst(GenTree* tree)

case TYP_FLOAT:
{
tree->gtVNPair.SetBoth(vnStore->VNForFloatCon((float)tree->AsDblCon()->DconValue()));
#ifdef TARGET_RISCV64
double f64Cns = tree->AsDblCon()->DconValue();
float f32Cns = *reinterpret_cast<float*>(&f64Cns);
if (!_isnan(f32Cns))
{
f32Cns = (float)f64Cns;
}
#else
float f32Cns = (float)tree->AsDblCon()->DconValue();
#endif // TARGET_RISCV64
tree->gtVNPair.SetBoth(vnStore->VNForFloatCon(f32Cns));
break;
}

Expand Down