Skip to content

Commit

Permalink
[RISC-V] Fix dropping NaN sign and payload when importing float const…
Browse files Browse the repository at this point in the history
…ants (#93285)

RISC-V float-to-double conversion canonicalizes the constant NaN value. Use soft conversion to double.

Since gtNewDconNode is used in quite a few places, the offending conversion is silent (float to double), and most programmers are unaware of NaN propagation issues on uncommon platforms like RISC-V, introduce factory functions dedicated to float and double that do the necessary conversions and make unintended upcasts more difficult.

Also fix printing NaN constants in dumps.
  • Loading branch information
tomeksowi authored Oct 11, 2023
1 parent bcf72c0 commit 701bfc0
Show file tree
Hide file tree
Showing 8 changed files with 38 additions and 25 deletions.
6 changes: 2 additions & 4 deletions src/coreclr/jit/assertionprop.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2674,7 +2674,7 @@ GenTree* Compiler::optVNConstantPropOnTree(BasicBlock* block, GenTree* tree)

case TYP_DOUBLE:
// Same sized reinterpretation of bits to double
conValTree = gtNewDconNode(*(reinterpret_cast<double*>(&value)));
conValTree = gtNewDconNodeD(*(reinterpret_cast<double*>(&value)));
break;

default:
Expand Down Expand Up @@ -2725,9 +2725,7 @@ GenTree* Compiler::optVNConstantPropOnTree(BasicBlock* block, GenTree* tree)

case TYP_FLOAT:
// Same sized reinterpretation of bits to float
conValTree = gtNewDconNode(FloatingPointUtils::convertToDouble(
BitOperations::UInt32BitsToSingle((uint32_t)value)),
TYP_FLOAT);
conValTree = gtNewDconNodeF(BitOperations::UInt32BitsToSingle((uint32_t)value));
break;

case TYP_DOUBLE:
Expand Down
5 changes: 4 additions & 1 deletion src/coreclr/jit/compiler.h
Original file line number Diff line number Diff line change
Expand Up @@ -2512,7 +2512,10 @@ class Compiler

GenTree* gtNewLconNode(__int64 value);

GenTree* gtNewDconNode(double value, var_types type = TYP_DOUBLE);
GenTree* gtNewDconNodeF(float value);
GenTree* gtNewDconNodeD(double value);
GenTree* gtNewDconNode(float value, var_types type) = delete; // use gtNewDconNodeF instead
GenTree* gtNewDconNode(double value, var_types type);

GenTree* gtNewSconNode(int CPX, CORINFO_MODULE_HANDLE scpHandle);

Expand Down
6 changes: 4 additions & 2 deletions src/coreclr/jit/emit.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -8310,7 +8310,8 @@ void emitter::emitOutputDataSec(dataSecDsc* sec, BYTE* dst)
switch (dsc->dsDataType)
{
case TYP_FLOAT:
printf(" ; float %9.6g", (double)*reinterpret_cast<float*>(&dsc->dsCont));
printf(" ; float %9.6g",
FloatingPointUtils::convertToDouble(*reinterpret_cast<float*>(&dsc->dsCont)));
break;
case TYP_DOUBLE:
printf(" ; double %12.9g", *reinterpret_cast<double*>(&dsc->dsCont));
Expand Down Expand Up @@ -8456,7 +8457,8 @@ void emitter::emitDispDataSec(dataSecDsc* section, BYTE* dst)
case TYP_FLOAT:
assert(data->dsSize >= 4);
printf("\tdd\t%08llXh\t", (UINT64) * reinterpret_cast<uint32_t*>(&data->dsCont[i]));
printf("\t; %9.6g", *reinterpret_cast<float*>(&data->dsCont[i]));
printf("\t; %9.6g",
FloatingPointUtils::convertToDouble(*reinterpret_cast<float*>(&data->dsCont[i])));
i += 4;
break;

Expand Down
22 changes: 15 additions & 7 deletions src/coreclr/jit/gentree.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7638,11 +7638,19 @@ GenTree* Compiler::gtNewLconNode(__int64 value)
return node;
}

GenTree* Compiler::gtNewDconNode(double value, var_types type)
GenTree* Compiler::gtNewDconNodeF(float value)
{
GenTree* node = new (this, GT_CNS_DBL) GenTreeDblCon(value, type);
return gtNewDconNode(FloatingPointUtils::convertToDouble(value), TYP_FLOAT);
}

return node;
GenTree* Compiler::gtNewDconNodeD(double value)
{
return gtNewDconNode(value, TYP_DOUBLE);
}

GenTree* Compiler::gtNewDconNode(double value, var_types type)
{
return new (this, GT_CNS_DBL) GenTreeDblCon(value, type);
}

GenTree* Compiler::gtNewSconNode(int CPX, CORINFO_MODULE_HANDLE scpHandle)
Expand Down Expand Up @@ -7895,12 +7903,12 @@ GenTree* Compiler::gtNewGenericCon(var_types type, uint8_t* cnsVal)
case TYP_FLOAT:
{
READ_VALUE(float);
return gtNewDconNode(val, TYP_FLOAT);
return gtNewDconNodeF(val);
}
case TYP_DOUBLE:
{
READ_VALUE(double);
return gtNewDconNode(val);
return gtNewDconNodeD(val);
}
case TYP_REF:
{
Expand Down Expand Up @@ -7975,11 +7983,11 @@ GenTree* Compiler::gtNewConWithPattern(var_types type, uint8_t pattern)
case TYP_FLOAT:
float floatPattern;
memset(&floatPattern, pattern, sizeof(floatPattern));
return gtNewDconNode(floatPattern, TYP_FLOAT);
return gtNewDconNodeF(floatPattern);
case TYP_DOUBLE:
double doublePattern;
memset(&doublePattern, pattern, sizeof(doublePattern));
return gtNewDconNode(doublePattern);
return gtNewDconNodeD(doublePattern);
case TYP_REF:
case TYP_BYREF:
assert(pattern == 0);
Expand Down
9 changes: 6 additions & 3 deletions src/coreclr/jit/importer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6336,14 +6336,17 @@ void Compiler::impImportBlockCode(BasicBlock* block)
case CEE_LDC_R8:
cval.dblVal = getR8LittleEndian(codeAddr);
JITDUMP(" %#.17g", cval.dblVal);
impPushOnStack(gtNewDconNode(cval.dblVal), typeInfo(TYP_DOUBLE));
impPushOnStack(gtNewDconNodeD(cval.dblVal), typeInfo(TYP_DOUBLE));
break;

case CEE_LDC_R4:
cval.dblVal = getR4LittleEndian(codeAddr);
{
GenTree* dcon = gtNewDconNodeF(getR4LittleEndian(codeAddr));
cval.dblVal = dcon->AsDblCon()->DconValue();
impPushOnStack(dcon, typeInfo(TYP_DOUBLE));
JITDUMP(" %#.17g", cval.dblVal);
impPushOnStack(gtNewDconNode(cval.dblVal, TYP_FLOAT), typeInfo(TYP_DOUBLE));
break;
}

case CEE_LDSTR:
val = getU4LittleEndian(codeAddr);
Expand Down
9 changes: 4 additions & 5 deletions src/coreclr/jit/importercalls.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3821,7 +3821,7 @@ GenTree* Compiler::impIntrinsic(GenTree* newobjThis,
if (op1->IsIntegralConst())
{
float f32Cns = BitOperations::UInt32BitsToSingle((uint32_t)op1->AsIntConCommon()->IconValue());
retNode = gtNewDconNode(FloatingPointUtils::convertToDouble(f32Cns), TYP_FLOAT);
retNode = gtNewDconNodeF(f32Cns);
}
else
{
Expand All @@ -3840,7 +3840,7 @@ GenTree* Compiler::impIntrinsic(GenTree* newobjThis,
impPopStack();

int64_t i64Cns = op1->AsIntConCommon()->LngValue();
retNode = gtNewDconNode(*reinterpret_cast<double*>(&i64Cns));
retNode = gtNewDconNodeD(*reinterpret_cast<double*>(&i64Cns));
}
#if TARGET_64BIT
else
Expand Down Expand Up @@ -4169,15 +4169,14 @@ GenTree* Compiler::impSRCSUnsafeIntrinsic(NamedIntrinsic intrinsic,
if (toType == TYP_DOUBLE)
{
uint64_t u64Cns = static_cast<uint64_t>(op1->AsIntConCommon()->LngValue());
return gtNewDconNode(BitOperations::UInt64BitsToDouble(u64Cns), TYP_DOUBLE);
return gtNewDconNodeD(BitOperations::UInt64BitsToDouble(u64Cns));
}
else
{
assert(toType == TYP_FLOAT);

uint32_t u32Cns = static_cast<uint32_t>(op1->AsIntConCommon()->IconValue());
float f32Cns = BitOperations::UInt32BitsToSingle(u32Cns);
return gtNewDconNode(FloatingPointUtils::convertToDouble(f32Cns), TYP_FLOAT);
return gtNewDconNodeF(BitOperations::UInt32BitsToSingle(u32Cns));
}
}
// TODO-CQ: We should support this on 32-bit via decomposition
Expand Down
4 changes: 2 additions & 2 deletions src/coreclr/jit/lowerxarch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -8363,13 +8363,13 @@ void Lowering::TryFoldCnsVecForEmbeddedBroadcast(GenTreeHWIntrinsic* parentNode,
case TYP_FLOAT:
{
float scalar = static_cast<float>(childNode->gtSimdVal.f32[0]);
constScalar = comp->gtNewDconNode(scalar, simdBaseType);
constScalar = comp->gtNewDconNodeF(scalar);
break;
}
case TYP_DOUBLE:
{
double scalar = static_cast<double>(childNode->gtSimdVal.f64[0]);
constScalar = comp->gtNewDconNode(scalar, simdBaseType);
constScalar = comp->gtNewDconNodeD(scalar);
break;
}
case TYP_INT:
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/jit/utils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -579,7 +579,7 @@ unsigned dumpSingleInstr(const BYTE* const codeAddr, IL_OFFSET offs, const char*
break;

case ShortInlineR:
dOp = getR4LittleEndian(opcodePtr);
dOp = FloatingPointUtils::convertToDouble(getR4LittleEndian(opcodePtr));
goto FLT_OP;
case InlineR:
dOp = getR8LittleEndian(opcodePtr);
Expand Down

0 comments on commit 701bfc0

Please sign in to comment.