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

Fix some flag uses #34969

Merged
merged 2 commits into from
Apr 15, 2020
Merged

Fix some flag uses #34969

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
14ca9e2
Fix some flag uses
CarolEidt Apr 14, 2020
994710e
Fix type verification check
CarolEidt Apr 15, 2020
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
9 changes: 6 additions & 3 deletions src/coreclr/src/jit/importer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1288,9 +1288,12 @@ GenTree* Compiler::impAssignStructPtr(GenTree* destAddr,
src->gtType = genActualType(returnType);
call->gtType = src->gtType;

// !!! The destination could be on stack. !!!
// This flag will let us choose the correct write barrier.
destFlags = GTF_IND_TGTANYWHERE;
if ((destAddr->gtOper != GT_ADDR) || (destAddr->AsOp()->gtOp1->gtOper != GT_LCL_VAR))
{
// !!! The destination could be on stack. !!!
// This flag will let us choose the correct write barrier.
destFlags = GTF_IND_TGTANYWHERE;
}
}
}
else if (src->OperIsBlk())
Expand Down
3 changes: 2 additions & 1 deletion src/coreclr/src/jit/lclvars.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3794,7 +3794,8 @@ void Compiler::lvaMarkLclRefs(GenTree* tree, BasicBlock* block, Statement* stmt,
allowStructs || genActualType(varDsc->TypeGet()) == genActualType(tree->gtType) ||
(tree->gtType == TYP_BYREF && varDsc->TypeGet() == TYP_I_IMPL) ||
(tree->gtType == TYP_I_IMPL && varDsc->TypeGet() == TYP_BYREF) || (tree->gtFlags & GTF_VAR_CAST) ||
varTypeIsFloating(varDsc->TypeGet()) && varTypeIsFloating(tree->gtType));
(varTypeIsFloating(varDsc) && varTypeIsFloating(tree)) ||
(varTypeIsStruct(varDsc) == varTypeIsStruct(tree)));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are cases where we have a SIMD that we don't normalize when the lclVar is created because the basetype is "Canon". These were getting by before because of the bug in the importer where we were setting GTF_IND_TGT_ANYWHERE on the lclVar, which for a lclVar is GTF_VAR_CAST, so it passed the above check.
Note that prior to the introduction of SIMD types, struct types would always "match" here.
(Also, I think the allowStructs is just wrong and should be eliminated - we can have structs on all targets.)

Copy link
Member

Choose a reason for hiding this comment

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

How do we ever see SIMD base types being __Canon? I though these had to be value types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will say that this is a bit of a mystery to me - it only happens during crossgen, and I've only observed it on a GT_RET_EXPR (which is the importer case that was incorrectly setting the flag).


/* Remember the type of the reference */

Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/src/jit/lsra.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9443,7 +9443,7 @@ void LinearScan::lsraGetOperandString(GenTree* tree,
unsigned operandStringLength)
{
const char* lastUseChar = "";
if ((tree->gtFlags & GTF_VAR_DEATH) != 0)
if (tree->OperIsScalarLocal() && ((tree->gtFlags & GTF_VAR_DEATH) != 0))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just a fix to the dump - we shouldn't be checking last use on anything but lclVars.

{
lastUseChar = "*";
}
Expand Down
6 changes: 3 additions & 3 deletions src/coreclr/src/jit/treelifeupdater.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,9 +74,9 @@ void TreeLifeUpdater<ForCodeGen>::UpdateLifeVar(GenTree* tree)
}

// if it's a partial definition then variable "x" must have had a previous, original, site to be born.
bool isBorn = ((tree->gtFlags & GTF_VAR_DEF) != 0 && (tree->gtFlags & GTF_VAR_USEASG) == 0);
bool isDying = ((tree->gtFlags & GTF_VAR_DEATH) != 0);
bool spill = ((tree->gtFlags & GTF_SPILL) != 0);
bool isBorn = ((lclVarTree->gtFlags & GTF_VAR_DEF) != 0 && (lclVarTree->gtFlags & GTF_VAR_USEASG) == 0);
bool isDying = ((lclVarTree->gtFlags & GTF_VAR_DEATH) != 0);
bool spill = ((lclVarTree->gtFlags & GTF_SPILL) != 0);

// Since all tracked vars are register candidates, but not all are in registers at all times,
// we maintain two separate sets of variables - the total set of variables that are either
Expand Down