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

Jump to bottom

Disable folding of implementation-defined casts #53782

Merged
merged 11 commits into from
Jul 7, 2021
Merged

Disable folding of implementation-defined casts #53782

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
04f6198
Enhance the FloatOvfToInt2 test to exercise VN paths
SingleAccretion Jun 6, 2021
c8a2270
Enhance the FloatOvfToInt2 test to cover importer
SingleAccretion Jun 6, 2021
4256905
Disable the test on Mono
SingleAccretion Jun 6, 2021
56aba84
Re-enable tests disabled against #13651
SingleAccretion Jun 6, 2021
74103e1
Re-enable tests disabled against #51346
SingleAccretion Jun 6, 2021
68a09e5
Re-enable tests disabled against #47374
SingleAccretion Jun 6, 2021
81e0a34
Disable folding in gtFoldExprConst
SingleAccretion Jun 6, 2021
bb9e4aa
Disable folding in VN
SingleAccretion Jun 6, 2021
d13b563
Temporarily promote the test to Pri0
SingleAccretion Jun 6, 2021
d66f7f1
Reword the comment
SingleAccretion Jul 6, 2021
f48608f
Move the tests back to Pri1
SingleAccretion Jul 6, 2021
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
46 changes: 13 additions & 33 deletions src/coreclr/jit/gentree.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14460,46 +14460,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
Open in desktop
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));
SingleAccretion marked this conversation as resolved.
Show resolved Hide resolved

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))
SingleAccretion marked this conversation as resolved.
Show resolved Hide resolved
{
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
Open in desktop
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