Skip to content

JIT: Remove LclVarDsc::lvExactSize and LclVarDsc::lvSIMDType - #83307

Merged
jakobbotsch merged 5 commits into
dotnet:mainfrom
jakobbotsch:remove-lvExactSize
Mar 13, 2023
Merged

JIT: Remove LclVarDsc::lvExactSize and LclVarDsc::lvSIMDType#83307
jakobbotsch merged 5 commits into
dotnet:mainfrom
jakobbotsch:remove-lvExactSize

Conversation

@jakobbotsch

@jakobbotschjakobbotsch commented Mar 11, 2023

Copy link
Copy Markdown
Member

Turn these into functions.

LclVarDsc shrinks by 8 bytes on win-x64, diff of layout: https://www.diffchecker.com/YcUPNTpu

@ghostghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Mar 11, 2023
@ghost

Copy link
Copy Markdown

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch, @kunalspathak
See info in area-owners.md if you want to be subscribed.

Issue Details

Turn these into functions.

Author:jakobbotsch
Assignees:jakobbotsch
Labels:

area-CodeGen-coreclr

Milestone:-

@jakobbotsch

Copy link
Copy Markdown
MemberAuthor

Diffs look to be because we were forgetting to set lvSIMDType = 1 for this particular local:

lvaSIMDInitTempVarNum = lvaGrabTempWithImplicitUse(falseDEBUGARG("SIMDInitTempVar"));

That means it wasn't being aligned in the stack frame before.

@jakobbotsch

Copy link
Copy Markdown
MemberAuthor

Diffs. As mentioned above, diffs are due to some more alignment for some SIMD locals. It is also a slight TP regression (0.02% to 0.05% in minopts, 0.08% to 0.10% in FullOpts) to remove this since calling lvExactSize is more expensive. I think the trade off is worth it -- we drop 8 bytes from every LclVarDsc and we don't keep a redundant field around that needs to be kept up to date (in both #83036 and #83306 I had failures from forgetting to set this field for SIMD types).

cc @dotnet/jit-contrib

@jakobbotsch
jakobbotsch marked this pull request as ready for review March 13, 2023 10:02

@EgorBoEgorBo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, LclVarDsc is fairly sensitive to changes, e.g. I locally re-shuffled some fields in it and got -0.25% TP improvement while keeping size the same, will file a PR to cover TP regressions you hit

@jakobbotsch

jakobbotsch commented Mar 13, 2023

Copy link
Copy Markdown
MemberAuthor

cover TP regressions you hit

I think we can just ignore these kinds of TP regressions, that's what we've done previously. If the native compiler decides that it is more efficient to emit shifts/adds instead of imul, then we should trust in it. It just further shows that the methodology of counting instructions executed is not a silver bullet (not that we thought that before).

Edit: Ah, I just realized you probably meant the actual TP regressions from calling lvExactSize() ... :-)

@jakobbotsch
jakobbotsch merged commit 7381860 into dotnet:mainMar 13, 2023
@jakobbotsch
jakobbotsch deleted the remove-lvExactSize branch March 13, 2023 23:01
@ghostghost locked as resolved and limited conversation to collaborators Apr 13, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@jakobbotsch@EgorBo@tannergooding