Skip to content

Simplify arithmetic operations on registry and memory#125559

Open
pedrobsaila wants to merge 9 commits intodotnet:mainfrom
pedrobsaila:125300
Open

Simplify arithmetic operations on registry and memory#125559
pedrobsaila wants to merge 9 commits intodotnet:mainfrom
pedrobsaila:125300

Conversation

@pedrobsaila
Copy link
Contributor

@pedrobsaila pedrobsaila commented Mar 14, 2026 •
edited
Loading

Fixes #125300

Lacks tests, working on it

Before :

Program:AddReg(int):int (FullOpts):
       lea      eax, [rdi+2*rdi]
       ret      

Program:AddMem(byref):int (FullOpts):
       mov      eax, dword ptr [rdi]
       lea      ecx, [rax+rax]
       add      eax, ecx
       ret      

Program:SubReg(int):int (FullOpts):
       xor      eax, eax
       sub      eax, edi
       ret      

Program:SubMem(byref):int (FullOpts):
       mov      eax, dword ptr [rdi]
       mov      ecx, eax
       sub      ecx, eax
       sub      ecx, eax
       mov      eax, ecx
       ret

After:

Program:AddReg(int):int (FullOpts):
       lea      eax, [rdi+2*rdi]
       ret      

Program:AddMem(byref):int (FullOpts):
       mov      eax, dword ptr [rdi]
       lea      eax, [rax+2*rax]
       ret      

Program:SubReg(int):int (FullOpts):
       mov      eax, ecx
       neg      eax
       ret      

Program:SubMem(byref):int (FullOpts):
       mov      eax, dword ptr [rdi]
       neg      eax
       ret

@github-actions github-actions bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Mar 14, 2026
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Mar 14, 2026
@dotnet-policy-service
Copy link
Contributor

dotnet-policy-service bot commented Mar 14, 2026

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

@EgorBo
Copy link
Member

EgorBo commented Mar 14, 2026 •
edited
Loading

haven't checked the correctness of these changes, but the diffs imply that +300 LOC to JIT are not worth it.

@pedrobsaila
Copy link
Contributor Author

pedrobsaila commented Mar 14, 2026 •
edited
Loading

There is room to reduce LOC, I just wrote it fast to see whether their will be asm diffs or not. I'll roll more compact version in a few hours. But I agree the diffs are not that impressive.

This was referenced Mar 14, 2026
Open
Open
@EgorBo
Copy link
Member

EgorBo commented Mar 14, 2026 •
edited
Loading

There is room to reduce LOC, I just wrote it fast to see whether their will be asm diffs or not. I'll roll more compact version in a few hours. But I agree the diffs are not that impressive.

Right. Generally, we don't accept PRs like that unless there is a strong justification. We see it as an extra risk that is not worth the impact. To be honest, all kinds of x + x + x .. transformation should be handled elsewhere, on VN/SSA level.

This was referenced Mar 14, 2026
Open
Closed
@pedrobsaila
Copy link
Contributor Author

pedrobsaila commented Mar 14, 2026

To be honest, all kinds of x + x + x .. transformation should be handled elsewhere, on VN/SSA level.

Sorry to bother you, but never worked on VN/SSA level, I need to ask you about which file the transformation should be valuenum.cpp/ssabuilder.cpp or elsewhere ?

@EgorBo
Copy link
Member

EgorBo commented Mar 14, 2026

To be honest, all kinds of x + x + x .. transformation should be handled elsewhere, on VN/SSA level.

Sorry to bother you, but never worked on VN/SSA level, I need to ask you about which file the transformation should be valuenum.cpp/ssabuilder.cpp or elsewhere ?

I just tried myself with VN and it didn't find diffs either (mostly because we don't materialize VNs today). With SSA it's probably a more advanced task. probably like in copyprop or assertionprop, but it might also not worth it - hard to tell in advance.

This was referenced Mar 14, 2026
Open
Open
Open
@tannergooding
Copy link
Member

tannergooding commented Mar 16, 2026

Do we have any info on whether the lack of diffs is because this is pattern is uncommon in all code or simply uncommon in the code that we have SPMI diffs around (i.e. corelib and partner frameworks/apps that tend to be a bit more explicit about our patterns)?

@pedrobsaila pedrobsaila mentioned this pull request Mar 21, 2026
Open
@pedrobsaila
Copy link
Contributor Author

pedrobsaila commented Mar 21, 2026

The ASM diffs seems better when transformation is moved to SSA (assertionprop.cpp), see #125885

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

Reviewers

No reviews

Assignees

No one assigned

Labels

area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

JIT: Simplify arithmetic operations on memory, as is already done for register

3 participants

@pedrobsaila @EgorBo @tannergooding