Skip to content

Commit 34e6bb4

Browse files
Fix a 32-bit specific bug in fgMorphCast (#52828)
* Added a test Verifying that checked '.un' casts from floats to small types are not treated as casts from unsigned types. * Do not mark casts from FP types with GTF_UNSIGNED It used to be that in the importer, whether the cast was to be marked as GTF_UNSIGNED was decided exclusively based on the incoming opcode. However, the flag only makes sense for casts from integral sources, and it turns out morph had a bug where it failed to clear this flag which resulted in bad codegen. The bug went as follows: "gtMorphCast" turns casts from an FP type to a small integer into a chain of casts: CAST(small integer <- FP) => CAST(small integer <- CAST(TYP_INT <- FP)). On 32 bit platforms, the code failed to clear the GTF_UNSIGNED flag from the original tree, which meant that the outer cast thought it had TYP_UINT as the source. This matters for checked casts: conv.ovf.i2.un(-2.0d), which is a legitimate conversion, was interpreted wrongly as an overflowing one as the resulting codegen only checked the upper bound via an unsigned compare. The fix is two-fold: clear GTF_UNSIGNED for GT_CAST nodes with FP sources on creation and unify the 64 bit and 32 bit paths in "gtMorphCast", which, after the removal of GTF_UNSIGNED handling, are identical. This is a zero-diff change across all SPMI collections for Windows x64, Linux x64, Linux ARM64. This **is not** a zero-diff change for Windows x86. Instances of bad codegen have been corrected in some tests. * Assert instead of normalizing Instead of normalizing GTF_UNSIGNED for FP sources in "gtNewCastNode", assert that it is not set in GenTreeCast's constructor and fix the importer to respect that constraint.
1 parent 7ab62de commit 34e6bb4

File tree

7 files changed

+448
-16
lines changed

7 files changed

+448
-16
lines changed

src/coreclr/jit/compiler.hpp

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1365,8 +1365,9 @@ inline void GenTree::SetOper(genTreeOps oper, ValueNumberUpdate vnUpdate)
13651365

13661366
inline GenTreeCast* Compiler::gtNewCastNode(var_types typ, GenTree* op1, bool fromUnsigned, var_types castType)
13671367
{
1368-
GenTreeCast* res = new (this, GT_CAST) GenTreeCast(typ, op1, fromUnsigned, castType);
1369-
return res;
1368+
GenTreeCast* cast = new (this, GT_CAST) GenTreeCast(typ, op1, fromUnsigned, castType);
1369+
1370+
return cast;
13701371
}
13711372

13721373
inline GenTreeCast* Compiler::gtNewCastNodeL(var_types typ, GenTree* op1, bool fromUnsigned, var_types castType)
@@ -1378,9 +1379,10 @@ inline GenTreeCast* Compiler::gtNewCastNodeL(var_types typ, GenTree* op1, bool f
13781379

13791380
/* Make a big node first and then change it to be GT_CAST */
13801381

1381-
GenTreeCast* res =
1382+
GenTreeCast* cast =
13821383
new (this, LargeOpOpcode()) GenTreeCast(typ, op1, fromUnsigned, castType DEBUGARG(/*largeNode*/ true));
1383-
return res;
1384+
1385+
return cast;
13841386
}
13851387

13861388
inline GenTreeIndir* Compiler::gtNewMethodTableLookup(GenTree* object)

src/coreclr/jit/gentree.h

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3531,6 +3531,11 @@ struct GenTreeCast : public GenTreeOp
35313531
GenTreeCast(var_types type, GenTree* op, bool fromUnsigned, var_types castType DEBUGARG(bool largeNode = false))
35323532
: GenTreeOp(GT_CAST, type, op, nullptr DEBUGARG(largeNode)), gtCastType(castType)
35333533
{
3534+
// We do not allow casts from floating point types to be treated as from
3535+
// unsigned to avoid bugs related to wrong GTF_UNSIGNED in case the
3536+
// CastOp's type changes.
3537+
assert(!varTypeIsFloating(op) || !fromUnsigned);
3538+
35343539
gtFlags |= fromUnsigned ? GTF_UNSIGNED : GTF_EMPTY;
35353540
}
35363541
#if DEBUGGABLE_GENTREE

src/coreclr/jit/importer.cpp

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13456,11 +13456,17 @@ void Compiler::impImportBlockCode(BasicBlock* block)
1345613456
callNode = varTypeIsFloating(impStackTop().val->TypeGet());
1345713457
}
1345813458

13459-
// At this point uns, ovf, callNode all set
13460-
1346113459
op1 = impPopStack().val;
1346213460
impBashVarAddrsToI(op1);
1346313461

13462+
// Casts from floating point types must not have GTF_UNSIGNED set.
13463+
if (varTypeIsFloating(op1))
13464+
{
13465+
uns = false;
13466+
}
13467+
13468+
// At this point uns, ovf, callNode are all set.
13469+
1346413470
if (varTypeIsSmall(lclTyp) && !ovfl && op1->gtType == TYP_INT && op1->gtOper == GT_AND)
1346513471
{
1346613472
op2 = op1->AsOp()->gtOp2;

src/coreclr/jit/morph.cpp

Lines changed: 5 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -183,20 +183,15 @@ GenTree* Compiler::fgMorphCast(GenTree* tree)
183183
// do we need to do it in two steps R -> I, '-> smallType
184184
CLANG_FORMAT_COMMENT_ANCHOR;
185185

186-
#if defined(TARGET_ARM64) || defined(TARGET_AMD64)
187186
if (dstSize < genTypeSize(TYP_INT))
188187
{
189-
oper = gtNewCastNodeL(TYP_INT, oper, tree->IsUnsigned(), TYP_INT);
190-
oper->gtFlags |= (tree->gtFlags & (GTF_OVERFLOW | GTF_EXCEPT));
191-
tree->gtFlags &= ~GTF_UNSIGNED;
192-
}
193-
#else
194-
if (dstSize < TARGET_POINTER_SIZE)
195-
{
196-
oper = gtNewCastNodeL(TYP_I_IMPL, oper, false, TYP_I_IMPL);
188+
oper = gtNewCastNodeL(TYP_INT, oper, /* fromUnsigned */ false, TYP_INT);
197189
oper->gtFlags |= (tree->gtFlags & (GTF_OVERFLOW | GTF_EXCEPT));
190+
// We must not mistreat the original cast, which was from a floating point type,
191+
// as from an unsigned type, since we now have a TYP_INT node for the source and
192+
// CAST_OVF(BYTE <- INT) != CAST_OVF(BYTE <- UINT).
193+
assert(!tree->IsUnsigned());
198194
}
199-
#endif
200195
else
201196
{
202197
/* Note that if we need to use a helper call then we can not morph oper */

0 commit comments

Comments
 (0)