Skip to content

Commit

Permalink
Disable folding of implementation-defined casts (#53782)
Browse files Browse the repository at this point in the history 
* Enhance the FloatOvfToInt2 test to exercise VN paths

It is not very effective right now because the "bad" combinations of
types are morphed into helpers too early, but it will be helpful when
we enable folding outside of the importer for those cases too.

* Enhance the FloatOvfToInt2 test to cover importer

It now fails on Windows x86, where many of the
previous similar failures were observed.

* Disable the test on Mono

* Re-enable tests disabled against #13651

* Re-enable tests disabled against #51346

* Re-enable tests disabled against #47374

* Disable folding in gtFoldExprConst

* Disable folding in VN

* Temporarily promote the test to Pri0

* Reword the comment

* Move the tests back to Pri1

Where they originally were.
  • Loading branch information
@SingleAccretion
SingleAccretion authored Jul 7, 2021
1 parent 974e1af commit 527e566
Show file tree
Hide file tree
Showing 5 changed files with 532 additions and 64 deletions.
46 changes: 13 additions & 33 deletions src/coreclr/jit/gentree.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14458,46 +14458,26 @@ GenTree* Compiler::gtFoldExprConst(GenTree* tree)
break;

case GT_CAST:
f1 = forceCastToFloat(d1);

if (tree->gtOverflow() &&
((op1->TypeIs(TYP_DOUBLE) && CheckedOps::CastFromDoubleOverflows(d1, tree->CastToType())) ||
(op1->TypeIs(TYP_FLOAT) &&
CheckedOps::CastFromFloatOverflows(forceCastToFloat(d1), tree->CastToType()))))
if ((op1->TypeIs(TYP_DOUBLE) && CheckedOps::CastFromDoubleOverflows(d1, tree->CastToType())) ||
(op1->TypeIs(TYP_FLOAT) && CheckedOps::CastFromFloatOverflows(f1, tree->CastToType())))
{
return tree;
}
// The conversion overflows. The ECMA spec says, in III 3.27, that
// "...if overflow occurs converting a floating point type to an integer, ...,
// the value returned is unspecified." However, it would at least be
// desirable to have the same value returned for casting an overflowing
// constant to an int as would be obtained by passing that constant as
// a parameter and then casting that parameter to an int type.

assert(tree->TypeIs(genActualType(tree->CastToType())));
// Don't fold overflowing converions, as the value returned by
// JIT's codegen doesn't always match with the C compiler's cast result.
// We want the behavior to be the same with or without folding.

if ((op1->TypeIs(TYP_FLOAT) && !_finite(forceCastToFloat(d1))) ||
(op1->TypeIs(TYP_DOUBLE) && !_finite(d1)))
{
// The floating point constant is not finite. The ECMA spec says, in
// III 3.27, that "...if overflow occurs converting a floating point type
// to an integer, ..., the value returned is unspecified." However, it would
// at least be desirable to have the same value returned for casting an overflowing
// constant to an int as would obtained by passing that constant as a parameter
// then casting that parameter to an int type. We will assume that the C compiler's
// cast logic will yield the desired result (and trust testing to tell otherwise).
// Cross-compilation is an issue here; if that becomes an important scenario, we should
// capture the target-specific values of overflow casts to the various integral types as
// constants in a target-specific function.
CLANG_FORMAT_COMMENT_ANCHOR;

// Don't fold conversions of +inf/-inf to integral value on all platforms
// as the value returned by JIT helper doesn't match with the C compiler's cast result.
// We want the behavior to be same with or without folding.
return tree;
}

if (d1 <= -1.0 && varTypeIsUnsigned(tree->CastToType()))
{
// Don't fold conversions of these cases becasue the result is unspecified per ECMA spec
// and the native math doing the fold doesn't match the run-time computation on all
// platforms.
// We want the behavior to be same with or without folding.
return tree;
}
assert(tree->TypeIs(genActualType(tree->CastToType())));

switch (tree->CastToType())
{
Expand Down
42 changes: 24 additions & 18 deletions src/coreclr/jit/valuenum.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3012,7 +3012,7 @@ ValueNum ValueNumStore::EvalCastForConstantArgs(var_types typ, VNFunc func, Valu
case TYP_FLOAT:
{
float arg0Val = GetConstantSingle(arg0VN);
assert(!checkedCast || !CheckedOps::CastFromFloatOverflows(arg0Val, castToType));
assert(!CheckedOps::CastFromFloatOverflows(arg0Val, castToType));

switch (castToType)
{
Expand Down Expand Up @@ -3054,7 +3054,7 @@ ValueNum ValueNumStore::EvalCastForConstantArgs(var_types typ, VNFunc func, Valu
case TYP_DOUBLE:
{
double arg0Val = GetConstantDouble(arg0VN);
assert(!checkedCast || !CheckedOps::CastFromDoubleOverflows(arg0Val, castToType));
assert(!CheckedOps::CastFromDoubleOverflows(arg0Val, castToType));

switch (castToType)
{
Expand Down Expand Up @@ -3322,26 +3322,32 @@ bool ValueNumStore::VNEvalShouldFold(var_types typ, VNFunc func, ValueNum arg0VN
}
}

// Is this a checked cast that will always throw an exception?
if (func == VNF_CastOvf)
// Is this a checked cast that will always throw an exception or one with an implementation-defined result?
if (VNFuncIsNumericCast(func))
{
var_types castToType;
bool fromUnsigned;
GetCastOperFromVN(arg1VN, &castToType, &fromUnsigned);
var_types castFromType = TypeOfVN(arg0VN);

switch (castFromType)
// By policy, we do not fold conversions from floating-point types that result in
// overflow, as the value the C++ compiler gives us does not always match our own codegen.
if ((func == VNF_CastOvf) || varTypeIsFloating(castFromType))
{
case TYP_INT:
return !CheckedOps::CastFromIntOverflows(GetConstantInt32(arg0VN), castToType, fromUnsigned);
case TYP_LONG:
return !CheckedOps::CastFromLongOverflows(GetConstantInt64(arg0VN), castToType, fromUnsigned);
case TYP_FLOAT:
return !CheckedOps::CastFromFloatOverflows(GetConstantSingle(arg0VN), castToType);
case TYP_DOUBLE:
return !CheckedOps::CastFromDoubleOverflows(GetConstantDouble(arg0VN), castToType);
default:
return false;
var_types castToType;
bool fromUnsigned;
GetCastOperFromVN(arg1VN, &castToType, &fromUnsigned);

switch (castFromType)
{
case TYP_INT:
return !CheckedOps::CastFromIntOverflows(GetConstantInt32(arg0VN), castToType, fromUnsigned);
case TYP_LONG:
return !CheckedOps::CastFromLongOverflows(GetConstantInt64(arg0VN), castToType, fromUnsigned);
case TYP_FLOAT:
return !CheckedOps::CastFromFloatOverflows(GetConstantSingle(arg0VN), castToType);
case TYP_DOUBLE:
return !CheckedOps::CastFromDoubleOverflows(GetConstantDouble(arg0VN), castToType);
default:
return false;
}
}
}

Expand Down
13 changes: 0 additions & 13 deletions src/libraries/System.Linq.Expressions/tests/Convert/ConvertTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1650,7 +1650,6 @@ public static void ConvertDoubleToNullableShortTest(bool useInterpreter)
}

[Theory, ClassData(typeof(CompilationTypes))]
[ActiveIssue("https://github.com/dotnet/runtime/issues/13651")]
public static void ConvertDoubleToUIntTest(bool useInterpreter)
{
foreach (double value in new double[] { 0, 1, -1, double.MinValue, double.MaxValue, double.Epsilon, double.NegativeInfinity, double.PositiveInfinity, double.NaN })
Expand All @@ -1660,7 +1659,6 @@ public static void ConvertDoubleToUIntTest(bool useInterpreter)
}

[Theory, ClassData(typeof(CompilationTypes))]
[ActiveIssue("https://github.com/dotnet/runtime/issues/13651")]
public static void ConvertDoubleToNullableUIntTest(bool useInterpreter)
{
foreach (double value in new double[] { 0, 1, -1, double.MinValue, double.MaxValue, double.Epsilon, double.NegativeInfinity, double.PositiveInfinity, double.NaN })
Expand All @@ -1679,7 +1677,6 @@ public static void ConvertDoubleToULongTest(bool useInterpreter)
}

[Theory, ClassData(typeof(CompilationTypes))]
[ActiveIssue("https://github.com/dotnet/runtime/issues/51346", TestRuntimes.CoreCLR)]
public static void ConvertDoubleToNullableULongTest(bool useInterpreter)
{
foreach (double value in new double[] { 0, 1, -1, double.MinValue, double.MaxValue, double.Epsilon, double.NegativeInfinity, double.PositiveInfinity, double.NaN })
Expand Down Expand Up @@ -1905,7 +1902,6 @@ public static void ConvertNullableDoubleToNullableShortTest(bool useInterpreter)
}

[Theory, ClassData(typeof(CompilationTypes))]
[ActiveIssue("https://github.com/dotnet/runtime/issues/51346", TestRuntimes.CoreCLR)]
public static void ConvertNullableDoubleToUIntTest(bool useInterpreter)
{
foreach (double? value in new double?[] { null, 0, 1, -1, double.MinValue, double.MaxValue, double.Epsilon, double.NegativeInfinity, double.PositiveInfinity, double.NaN })
Expand All @@ -1915,7 +1911,6 @@ public static void ConvertNullableDoubleToUIntTest(bool useInterpreter)
}

[Theory, ClassData(typeof(CompilationTypes))]
[ActiveIssue("https://github.com/dotnet/runtime/issues/51346", TestRuntimes.CoreCLR)]
public static void ConvertNullableDoubleToNullableUIntTest(bool useInterpreter)
{
foreach (double? value in new double?[] { null, 0, 1, -1, double.MinValue, double.MaxValue, double.Epsilon, double.NegativeInfinity, double.PositiveInfinity, double.NaN })
Expand All @@ -1934,7 +1929,6 @@ public static void ConvertNullableDoubleToULongTest(bool useInterpreter)
}

[Theory, ClassData(typeof(CompilationTypes))]
[ActiveIssue("https://github.com/dotnet/runtime/issues/51346", TestRuntimes.CoreCLR)]
public static void ConvertNullableDoubleToNullableULongTest(bool useInterpreter)
{
foreach (double? value in new double?[] { null, 0, 1, -1, double.MinValue, double.MaxValue, double.Epsilon, double.NegativeInfinity, double.PositiveInfinity, double.NaN })
Expand Down Expand Up @@ -3096,7 +3090,6 @@ public static void ConvertFloatToNullableShortTest(bool useInterpreter)
}

[Theory, ClassData(typeof(CompilationTypes))]
[ActiveIssue("https://github.com/dotnet/runtime/issues/47374", TestRuntimes.CoreCLR)]
public static void ConvertFloatToUIntTest(bool useInterpreter)
{
foreach (float value in new float[] { 0, 1, -1, float.MinValue, float.MaxValue, float.Epsilon, float.NegativeInfinity, float.PositiveInfinity, float.NaN })
Expand All @@ -3106,7 +3099,6 @@ public static void ConvertFloatToUIntTest(bool useInterpreter)
}

[Theory, ClassData(typeof(CompilationTypes))]
[ActiveIssue("https://github.com/dotnet/runtime/issues/47374", TestRuntimes.CoreCLR)]
public static void ConvertFloatToNullableUIntTest(bool useInterpreter)
{
foreach (float value in new float[] { 0, 1, -1, float.MinValue, float.MaxValue, float.Epsilon, float.NegativeInfinity, float.PositiveInfinity, float.NaN })
Expand All @@ -3124,9 +3116,7 @@ public static void ConvertFloatToULongTest(bool useInterpreter)
}
}


[Theory, ClassData(typeof(CompilationTypes))]
[ActiveIssue("https://github.com/dotnet/runtime/issues/51346", TestRuntimes.CoreCLR)]
public static void ConvertFloatToNullableULongTest(bool useInterpreter)
{
foreach (float value in new float[] { 0, 1, -1, float.MinValue, float.MaxValue, float.Epsilon, float.NegativeInfinity, float.PositiveInfinity, float.NaN })
Expand Down Expand Up @@ -3352,7 +3342,6 @@ public static void ConvertNullableFloatToNullableShortTest(bool useInterpreter)
}

[Theory, ClassData(typeof(CompilationTypes))]
[ActiveIssue("https://github.com/dotnet/runtime/issues/51346", TestRuntimes.CoreCLR)]
public static void ConvertNullableFloatToUIntTest(bool useInterpreter)
{
foreach (float? value in new float?[] { null, 0, 1, -1, float.MinValue, float.MaxValue, float.Epsilon, float.NegativeInfinity, float.PositiveInfinity, float.NaN })
Expand All @@ -3362,7 +3351,6 @@ public static void ConvertNullableFloatToUIntTest(bool useInterpreter)
}

[Theory, ClassData(typeof(CompilationTypes))]
[ActiveIssue("https://github.com/dotnet/runtime/issues/51346", TestRuntimes.CoreCLR)]
public static void ConvertNullableFloatToNullableUIntTest(bool useInterpreter)
{
foreach (float? value in new float?[] { null, 0, 1, -1, float.MinValue, float.MaxValue, float.Epsilon, float.NegativeInfinity, float.PositiveInfinity, float.NaN })
Expand All @@ -3381,7 +3369,6 @@ public static void ConvertNullableFloatToULongTest(bool useInterpreter)
}

[Theory, ClassData(typeof(CompilationTypes))]
[ActiveIssue("https://github.com/dotnet/runtime/issues/51346", TestRuntimes.CoreCLR)]
public static void ConvertNullableFloatToNullableULongTest(bool useInterpreter)
{
foreach (float? value in new float?[] { null, 0, 1, -1, float.MinValue, float.MaxValue, float.Epsilon, float.NegativeInfinity, float.PositiveInfinity, float.NaN })
Expand Down
Loading

0 comments on commit 527e566

Please sign in to comment.