Skip to content

Delete an unnecessary pessimization for x86. #52803

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.

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
merged 1 commit into from
May 21, 2021
Merged

Conversation

sandreenko
Copy link
Contributor

@sandreenko sandreenko commented May 15, 2021

Now if we have an incoming struct its fields can be enregistered on x86.
It includes both cases:

  1. the struct is coming on stack but we copy it to regs;
  2. the struct is coming on reg, in x86 case it is only possible for something like struct {int} and the field is used from the reg.
benchmarks.run.windows.x86
  Total bytes of delta: -2195 (-1.25% of base)
  440 total files with Code Size differences (340 improved, 100 regressed), 120 unchanged.
libraries.crossgen.windows.x86
  Total bytes of delta: -10702 (-1.71% of base)
  2274 total files with Code Size differences (1827 improved, 447 regressed), 1043 unchanged.
libraries.crossgen2.windows.x86
  Total bytes of delta: -5375 (-3.53% of base)
  938 total files with Code Size differences (771 improved, 167 regressed), 169 unchanged.
libraries.pmi.windows.x86
  Total bytes of delta: -18612 (-1.50% of base)
  3752 total files with Code Size differences (2828 improved, 924 regressed), 1593 unchanged.
tests.pmi.windows.x86
  Total bytes of delta: -3563 (-6.94% of base)
  419 total files with Code Size differences (380 improved, 39 regressed), 59 unchanged.
tests_libraries.pmi.windows.x86
  Total bytes of delta: -3892 (-0.61% of base)
  1220 total files with Code Size differences (890 improved, 330 regressed), 1705 unchanged.

the regressions are from additional reg pressure + CSE, nothing stands up.
libraries diffs are very nice, hope some people are still using x86 :-)

I will do it for arm32 as well, this condition was breaking a struct contract that I need to satisfy for struct enreg, the contract is:
if struct's promotion type is 'DEPENDENT' the struct should be marked as 'doNotEnreg`.

@sandreenko sandreenko added arch-x86 area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI labels May 15, 2021
@sandreenko sandreenko marked this pull request as draft May 16, 2021 09:39
@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 May 19, 2021 06:33
@sandreenko
Copy link
Contributor Author

A diff example:

base:
2c415d0: 50                     push    eax
2c415d1: 89 14 24               mov     dword ptr [esp], edx
2c415d4: 8b 04 24               mov     eax, dword ptr [esp]
2c415d7: 89 01                  mov     dword ptr [ecx], eax
2c415d9: 59                     pop     ecx
2c415da: c3                     ret

diff:
2c41730: 89 11                  mov     dword ptr [ecx], edx
2c41732: c3                     ret

@sandreenko
Copy link
Contributor Author

The failures are due to #52988 (unrelated).

@sandreenko
Copy link
Contributor Author

PTAL @dotnet/jit-contrib

Copy link
Member

@AndyAyersMS AndyAyersMS left a comment

Choose a reason for hiding this comment

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

LGTM

@sandreenko sandreenko merged commit 77f0728 into dotnet:main May 21, 2021
@sandreenko sandreenko deleted the x86opt branch May 21, 2021 03:27
@ghost ghost locked as resolved and limited conversation to collaborators Jun 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-x86 area-CodeGen-coreclr CLR 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.

2 participants