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

Prepare JIT backend for structs in registers. #52039

Merged
merged 5 commits into from
May 5, 2021
Merged

Prepare JIT backend for structs in registers. #52039

merged 5 commits into from
May 5, 2021

Conversation

sandreenko
Copy link
Contributor

@sandreenko sandreenko commented Apr 29, 2021 •
edited
Loading

The PR prepares our backend to see LCL_VAR struct in registers, part of #43867.
It has 0 diffs as expected because JitEnregStructLocals is set to 0.

The main change is around spilling/reloading code because for direct usages we replace "struct" type with platform-specific type during lowering (like for "calls()" and "returns").

@sandreenko sandreenko added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Apr 29, 2021
@sandreenko sandreenko self-assigned this Apr 29, 2021
@sandreenko
Copy link
Contributor Author

/azp run runtime-coreclr jitstress

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@sandreenko
Copy link
Contributor Author

/azp run runtime-coreclr jitstress

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@sandreenko sandreenko marked this pull request as ready for review April 30, 2021 17:07
@sandreenko
Copy link
Contributor Author

PTAL @BruceForstall @dotnet/jit-contrib

@kunalspathak
Copy link
Member

kunalspathak commented May 1, 2021 •
edited
Loading

So this change is only for xarch? Any changes this (or next PRs) will help handle one of these problems in Arm64 - #41704 (comment)?

BruceForstall
BruceForstall reviewed May 1, 2021
View reviewed changes
Copy link
Member

@BruceForstall BruceForstall left a comment

Choose a reason for hiding this comment

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

A few questions.

Did you run x86 diffs, too?

src/coreclr/jit/jitconfigvalues.h
@@ -517,6 +517,9 @@ CONFIG_INTEGER(JitSaveFpLrWithCalleeSavedRegisters, W("JitSaveFpLrWithCalleeSave
#endif // defined(TARGET_ARM64)
#endif // DEBUG

// Allow to enregister locals with struct type.
CONFIG_INTEGER(JitEnregStructLocals, W("JitEnregStructLocals"), 0)
Copy link
Member

Choose a reason for hiding this comment

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

You want this to be available in Release?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

src/coreclr/jit/lclvars.cpp Outdated
// tree - node that uses the local, its type is checked first.
//
// Return Value:
// TYP_UNDEF if the layout is enregistrable, register type otherwise.
Copy link
Member

Choose a reason for hiding this comment

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

Isn't it the opposite? That is, return TYP_UNDEF if the layout is NOT enregistrable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, fixed.

src/coreclr/jit/lclvars.cpp Outdated
// Ensure that lclVar nodes are typed correctly.
if (tree->OperIs(GT_STORE_LCL_VAR) && lvNormalizeOnStore())
{
// TODO: update that assert to work with TypeGet() == TYP_STRUCT case.
Copy link
Member

Choose a reason for hiding this comment

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

Are you going to fix this "TODO" now, or later? Or should the whole block be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

kunalspathak
kunalspathak approved these changes May 3, 2021
View reviewed changes
Copy link
Member

@kunalspathak kunalspathak left a comment

Choose a reason for hiding this comment

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

LGTM overall

src/coreclr/jit/codegenlinear.cpp Outdated
@@ -864,14 +864,14 @@ void CodeGen::genSpillVar(GenTree* tree)
// therefore be store-normalized (rather than load-normalized). In fact, not performing store normalization
// can lead to problems on architectures where a lclVar may be allocated to a register that is not
// addressable at the granularity of the lclVar's defined type (e.g. x86).
var_types lclTyp = genActualType(varDsc->TypeGet());
emitAttr size = emitTypeSize(lclTyp);
var_types lclType = genActualType(varDsc->GetRegisterType());
Copy link
Member

Choose a reason for hiding this comment

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

If we are going to have lot of patterns like genActualType(varDsc->GetRegisterType()), probably worth adding an override to genActualType(varDsc) that does the GetRegisterType() part?

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 like this, added.

@sandreenko
Copy link
Contributor Author

For @BruceForstall :

Did you run x86 diffs, too?

Yes, no diffs as well.

For @kunalspathak :

So this change is only for xarch?

These changes are for all platforms.

Any changes this (or next PRs) will help handle one of these problems in Arm64.

The struct enreg will help arm64 but not the problems listed there.

BruceForstall
BruceForstall approved these changes May 4, 2021
View reviewed changes
src/coreclr/jit/compiler.h Outdated Show resolved Hide resolved
@sandreenko sandreenko merged commit 7c98f34 into dotnet:main May 5, 2021
@sandreenko sandreenko mentioned this pull request May 10, 2021
Closed
10 tasks
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
@JulieLeeMSFT JulieLeeMSFT mentioned this pull request Jun 8, 2021
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Jun 19, 2021
@AndyAyersMS
Copy link
Member

Starting to wonder if the widening we do for spilling is correct, see eg #67188, #67152, #66720.

(Also still quite annoying that auto-linking doesn't happen when a related issue is updated by a "collaborator", presumably because we've locked the PR).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@BruceForstall BruceForstall BruceForstall approved these changes

@kunalspathak kunalspathak kunalspathak approved these changes

Assignees

@sandreenko sandreenko

Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Milestone
6.0.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@sandreenko @kunalspathak @AndyAyersMS @BruceForstall @karelz