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

Merged
merged 4 commits into from
Jul 21, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
6 changes: 4 additions & 2 deletions src/coreclr/jit/assertionprop.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2587,7 +2587,7 @@ GenTree* Compiler::optVNConstantPropOnTree(BasicBlock* block, GenTree* tree)
{
// Implicit conversion to float or double
assert(varTypeIsFloating(tree->TypeGet()));
conValTree = gtNewDconNode(value, tree->TypeGet());
conValTree = gtNewDconNode(FloatingPointUtils::convertToDouble(value), tree->TypeGet());
}
break;
}
Expand Down Expand Up @@ -2697,7 +2697,9 @@ GenTree* Compiler::optVNConstantPropOnTree(BasicBlock* block, GenTree* tree)

case TYP_FLOAT:
// Same sized reinterpretation of bits to float
conValTree = gtNewDconNode(*reinterpret_cast<float*>(&value), TYP_FLOAT);
conValTree =
gtNewDconNode(FloatingPointUtils::convertToDouble(*reinterpret_cast<float*>(&value)),
TYP_FLOAT);
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
conValTree =
gtNewDconNode(FloatingPointUtils::convertToDouble(*reinterpret_cast<float*>(&value)),
TYP_FLOAT);
conValTree =
gtNewDconNode(FloatingPointUtils::convertToDouble(BitOperations::UInt32BitsToSingle((uint32_t)value)),
TYP_FLOAT);

break;

case TYP_DOUBLE:
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/jit/emit.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -8050,7 +8050,7 @@ CORINFO_FIELD_HANDLE emitter::emitFltOrDblConst(double constValue, emitAttr attr

if (attr == EA_4BYTE)
{
f = forceCastToFloat(constValue);
f = FloatingPointUtils::convertToSingle(constValue);
cnsAddr = &f;
dataType = TYP_FLOAT;
}
Expand Down
10 changes: 6 additions & 4 deletions src/coreclr/jit/importercalls.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3794,7 +3794,8 @@ 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);
float f32Cns = *reinterpret_cast<float*>(&i32Cns);
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
float f32Cns = *reinterpret_cast<float*>(&i32Cns);
float f32Cns = BitOperations::UInt32BitsToSingle((uint32_t)op1->AsIntConCommon()->IconValue());

Now that you're here...

retNode = gtNewDconNode(FloatingPointUtils::convertToDouble(f32Cns), TYP_FLOAT);
}
else
{
Expand Down Expand Up @@ -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));
Copy link
Member

@jakobbotsch jakobbotsch Jul 20, 2023

Choose a reason for hiding this comment

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

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

(since we're here anyways)

}
else
Expand Down Expand Up @@ -4141,7 +4142,7 @@ GenTree* Compiler::impSRCSUnsafeIntrinsic(NamedIntrinsic intrinsic,
else
{
assert(fromType == TYP_FLOAT);
float f32Cns = static_cast<float>(op1->AsDblCon()->DconValue());
float f32Cns = FloatingPointUtils::convertToSingle(op1->AsDblCon()->DconValue());
return gtNewIconNode(static_cast<int32_t>(BitOperations::SingleToUInt32Bits(f32Cns)));
}
}
Expand Down Expand Up @@ -4181,7 +4182,8 @@ 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);
float f32Cns = BitOperations::UInt32BitsToSingle(u32Cns);
return gtNewDconNode(FloatingPointUtils::convertToDouble(f32Cns), TYP_FLOAT);
}
}
else
Expand Down
64 changes: 64 additions & 0 deletions src/coreclr/jit/utils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2051,6 +2051,70 @@ unsigned __int64 FloatingPointUtils::convertDoubleToUInt64(double d)
return u64;
}

//------------------------------------------------------------------------
// 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) | 0x7FF8000000000000ul | ((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) | 0x7F800000u | newPayload;
return BitOperations::UInt32BitsToSingle(newBits);
#else
return (float)d;
#endif
}

// Rounds a double-precision floating-point value to the nearest integer,
// and rounds midpoint values to the nearest even number.
double FloatingPointUtils::round(double x)
Expand Down
4 changes: 4 additions & 0 deletions src/coreclr/jit/utils.h
Original file line number Diff line number Diff line change
Expand Up @@ -729,6 +729,10 @@ class FloatingPointUtils

static unsigned __int64 convertDoubleToUInt64(double d);

static double convertToDouble(float f);

static float convertToSingle(double d);

static double round(double x);

static float round(float x);
Expand Down
3 changes: 2 additions & 1 deletion src/coreclr/jit/valuenum.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -10345,7 +10345,8 @@ void Compiler::fgValueNumberTreeConst(GenTree* tree)

case TYP_FLOAT:
{
tree->gtVNPair.SetBoth(vnStore->VNForFloatCon((float)tree->AsDblCon()->DconValue()));
float f32Cns = FloatingPointUtils::convertToSingle(tree->AsDblCon()->DconValue());
tree->gtVNPair.SetBoth(vnStore->VNForFloatCon(f32Cns));
break;
}

Expand Down