Skip to content

Fix a 32-bit specific bug in fgMorphCast #52828

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

Merged
sandreenko merged 3 commits into from
May 24, 2021
Merged

Fix a 32-bit specific bug in fgMorphCast #52828

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9d9e769
Added a test
SingleAccretion May 16, 2021
6e87a65
Do not mark casts from FP types with GTF_UNSIGNED
SingleAccretion May 17, 2021
a1a0850
Assert instead of normalizing
SingleAccretion May 21, 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
10 changes: 6 additions & 4 deletions src/coreclr/jit/compiler.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1412,8 +1412,9 @@ inline void GenTree::SetOper(genTreeOps oper, ValueNumberUpdate vnUpdate)

inline GenTreeCast* Compiler::gtNewCastNode(var_types typ, GenTree* op1, bool fromUnsigned, var_types castType)
{
GenTreeCast* res = new (this, GT_CAST) GenTreeCast(typ, op1, fromUnsigned, castType);
return res;
GenTreeCast* cast = new (this, GT_CAST) GenTreeCast(typ, op1, fromUnsigned, castType);
Copy link
Contributor

@sandreenko sandreenko May 21, 2021

Choose a reason for hiding this comment

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

we are calling GenTreeCast constructor:

runtime/src/coreclr/jit/gentree.h

Lines 3531 to 3535 in 4197653

GenTreeCast(var_types type, GenTree* op, bool fromUnsigned, var_types castType DEBUGARG(bool largeNode = false))
: GenTreeOp(GT_CAST, type, op, nullptr DEBUGARG(largeNode)), gtCastType(castType)
{
gtFlags |= fromUnsigned ? GTF_UNSIGNED : GTF_EMPTY;
}

and after that we call CanonicalizeCastNode to clear the flag in some cases, is it worth it?
One option is to do

inline GenTreeCast* Compiler::gtNewCastNode(var_types typ, GenTree* op1, bool fromUnsigned, var_types castType)
{
  if (varTypeIsFloating(cast->CastOp())) { fromUnsigned = false; }
  GenTreeCast* cast = new (this, GT_CAST) GenTreeCast(typ, op1, fromUnsigned, castType);

or add this logic to GenTreeCast constructor (note it will affect more cases then).

Often I set conditional breakpoints on flag values so I would prefer not to change it back and forth without a reason.

Have you tried to add assert(!varTypeIsFloating(cast->CastOp() || !fromUnsigned) and fix the callers who call it with wrong value of bool fromUnsigned?

Copy link
Contributor Author

@SingleAccretion SingleAccretion May 21, 2021

Choose a reason for hiding this comment

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

Have you tried to add assert(!varTypeIsFloating(cast->CastOp() || !fromUnsigned) and fix the callers who call it with wrong value of bool fromUnsigned?

Yes, in fact that was the original version of the fix, and the only place I had to fix was the importer. I went back and forth on this a few times, and ultimately decided to have the change initially be with this more conservative implementation, because it is "guaranteed" (not really because I did not add it to the constructor, but nothing calls the constructor directly) to always return "correct" result to the caller. I suppose I should revisit it and run the assertty version through the CI. Maybe it turns out more callers are getting it wrong and it would be better to normalize centrally (regardless of that, the flag flipping will be fixed).

Copy link
Contributor

@sandreenko sandreenko May 21, 2021

Choose a reason for hiding this comment

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

I suppose I should revisit it and run the assertty version through the CI. Maybe it turns out more callers are getting it wrong and it would be better to normalize centrally (regardless of that, the flag flipping will be fixed).

I would prefer this if you have time, if not please open a follow-up issue to investigate/clean it and we will merge this PR as is.

Copy link
Contributor Author

@SingleAccretion SingleAccretion May 21, 2021

Choose a reason for hiding this comment

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

I would prefer this

I would as well, so we'll investigate it as part of this PR.


return cast;
}

inline GenTreeCast* Compiler::gtNewCastNodeL(var_types typ, GenTree* op1, bool fromUnsigned, var_types castType)
Expand All @@ -1425,9 +1426,10 @@ inline GenTreeCast* Compiler::gtNewCastNodeL(var_types typ, GenTree* op1, bool f

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

GenTreeCast* res =
GenTreeCast* cast =
new (this, LargeOpOpcode()) GenTreeCast(typ, op1, fromUnsigned, castType DEBUGARG(/*largeNode*/ true));
return res;

return cast;
}

inline GenTreeIndir* Compiler::gtNewMethodTableLookup(GenTree* object)
Expand Down
5 changes: 5 additions & 0 deletions src/coreclr/jit/gentree.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3531,6 +3531,11 @@ struct GenTreeCast : public GenTreeOp
GenTreeCast(var_types type, GenTree* op, bool fromUnsigned, var_types castType DEBUGARG(bool largeNode = false))
: GenTreeOp(GT_CAST, type, op, nullptr DEBUGARG(largeNode)), gtCastType(castType)
{
// We do not allow casts from floating point types to be treated as from
// unsigned to avoid bugs related to wrong GTF_UNSIGNED in case the
// CastOp's type changes.
assert(!varTypeIsFloating(op) || !fromUnsigned);

gtFlags |= fromUnsigned ? GTF_UNSIGNED : GTF_EMPTY;
}
#if DEBUGGABLE_GENTREE
Expand Down
10 changes: 8 additions & 2 deletions src/coreclr/jit/importer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13456,11 +13456,17 @@ void Compiler::impImportBlockCode(BasicBlock* block)
callNode = varTypeIsFloating(impStackTop().val->TypeGet());
}

// At this point uns, ovf, callNode all set

op1 = impPopStack().val;
impBashVarAddrsToI(op1);

// Casts from floating point types must not have GTF_UNSIGNED set.
if (varTypeIsFloating(op1))
{
uns = false;
}

// At this point uns, ovf, callNode are all set.

if (varTypeIsSmall(lclTyp) && !ovfl && op1->gtType == TYP_INT && op1->gtOper == GT_AND)
{
op2 = op1->AsOp()->gtOp2;
Expand Down
15 changes: 5 additions & 10 deletions src/coreclr/jit/morph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,20 +183,15 @@ GenTree* Compiler::fgMorphCast(GenTree* tree)
// do we need to do it in two steps R -> I, '-> smallType
CLANG_FORMAT_COMMENT_ANCHOR;

#if defined(TARGET_ARM64) || defined(TARGET_AMD64)
if (dstSize < genTypeSize(TYP_INT))
{
oper = gtNewCastNodeL(TYP_INT, oper, tree->IsUnsigned(), TYP_INT);
oper->gtFlags |= (tree->gtFlags & (GTF_OVERFLOW | GTF_EXCEPT));
tree->gtFlags &= ~GTF_UNSIGNED;
}
#else
if (dstSize < TARGET_POINTER_SIZE)
{
oper = gtNewCastNodeL(TYP_I_IMPL, oper, false, TYP_I_IMPL);
oper = gtNewCastNodeL(TYP_INT, oper, /* fromUnsigned */ false, TYP_INT);
oper->gtFlags |= (tree->gtFlags & (GTF_OVERFLOW | GTF_EXCEPT));
// We must not mistreat the original cast, which was from a floating point type,
// as from an unsigned type, since we now have a TYP_INT node for the source and
// CAST_OVF(BYTE <- INT) != CAST_OVF(BYTE <- UINT).
assert(!tree->IsUnsigned());
}
#endif
else
{
/* Note that if we need to use a helper call then we can not morph oper */
Expand Down
Loading