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

Fix some flag uses #34969

Merged
merged 2 commits into from
Apr 15, 2020
Merged

Fix some flag uses #34969

merged 2 commits into from
Apr 15, 2020

Conversation

CarolEidt
Copy link
Contributor

These issues were found while working on rearranging the flags for #34822.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Apr 14, 2020
@CarolEidt
Copy link
Contributor Author

These were accidental overlaps in flag usage between GT_IND and GT_LCL_VAR. Fixing these resulted in changes mostly due to copy propagation. In a few cases the resulting code was worse, but overall it was better. I did crossgen diffs for frameworks and tests on x64, x86, arm64, arm32, and x64/ux (for the related work that I'm doing on enregistering multireg vars superpmi diffs are problematic because the JIT asks more questions about those structs).

Windows x86 and x64 had zero diffs. The others were:

arm32: -180 (-0.00% of base) (4 improved, 1 regressed)
arm64: -1208 (-0.00% of base) (67 improved, 11 regressed)
x64ux: -1722 (-0.00% of base) (67 improved, 15 regressed)

CarolEidt
CarolEidt commented Apr 15, 2020
View reviewed changes
src/coreclr/src/jit/lclvars.cpp
@@ -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).

src/coreclr/src/jit/lsra.cpp
@@ -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.

@CarolEidt
Copy link
Contributor Author

@dotnet/jit-contrib PTAL

briansull
briansull approved these changes Apr 15, 2020
View reviewed changes
Copy link
Contributor

@briansull briansull left a comment

Choose a reason for hiding this comment

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

LGTM

@CarolEidt CarolEidt merged commit a6e9098 into dotnet:master Apr 15, 2020
@jaredpar jaredpar mentioned this pull request Apr 16, 2020
Closed
@CarolEidt CarolEidt mentioned this pull request Apr 16, 2020
Merged
@CarolEidt CarolEidt deleted the FixFlagUses branch July 16, 2020 17:01
@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@AndyAyersMS AndyAyersMS AndyAyersMS left review comments

@briansull briansull briansull approved these changes

Assignees
No one assigned
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@CarolEidt @AndyAyersMS @briansull @Dotnet-GitSync-Bot