Skip to content

[JIT] X64 - Allow containment of larger memory operands for AND, OR, XOR #79126

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
Merged
TIHan merged 1 commit into from
Dec 7, 2022
Merged

[JIT] X64 - Allow containment of larger memory operands for AND, OR, XOR #79126

TIHan merged 1 commit into from
Dec 7, 2022

Conversation

@TIHan
Copy link
Contributor

@TIHan TIHan commented Dec 2, 2022 •
edited
Loading

Description

This PR allows the operators, AND, OR, XOR to be able to contain memory operands that are of larger size than the operator itself. There is only a single case where we use a small type for those operators, it happens when trying to take advantage of CPU flags for a JCC on x86/x64.

This is needed to prevent a few regressions from this PR: #77874

Example diffs:

-       mov      eax, dword ptr [rsp+4A8H]
-       and      cl, al
+       and      cl, byte  ptr [rsp+4A8H]
-       mov      edi, dword ptr [rsp+1C0H]
-       or       dil, bl
+       or       bl, byte  ptr [rsp+1C0H]

Acceptance Criteria

  • CI passes

@ghost ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Dec 2, 2022
@ghost ghost assigned TIHan Dec 2, 2022
@ghost
Copy link

ghost commented Dec 2, 2022

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

Issue Details

** Description **

This PR allows the operators, AND, OR, XOR to be able to contain memory operands that are of larger size than the operator itself.

This is needed to prevent a few regressions from this PR: #77874

** Acceptance Criteria **

  • CI passes
Author: TIHan
Assignees: -
Labels:

area-CodeGen-coreclr
Milestone: -

@TIHan TIHan mentioned this pull request Dec 2, 2022
Merged
3 tasks
TIHan
TIHan commented Dec 2, 2022
View reviewed changes
src/coreclr/jit/lower.h

// Return true if 'childNode' is a containable memory op by its size relative to the 'parentNode'.
// Currently very conservative.
bool IsContainableMemoryOpSize(GenTree* parentNode, GenTree* childNode) const
Copy link
Contributor Author

@TIHan TIHan Dec 2, 2022

Choose a reason for hiding this comment

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

Not sure what to name this. IsContainableMemoryOpSize might be confusing considering we have a IsContainableMemoryOp already.

@TIHan TIHan requested a review from BruceForstall December 2, 2022 04:48
@TIHan
Copy link
Contributor Author

TIHan commented Dec 2, 2022

@dotnet/jit-contrib ready for review

@tannergooding
Copy link
Member

tannergooding commented Dec 6, 2022

Why just and/or/xor and not more generally?

We're on a little endian platform so it's safe to just access the lowest n-bits for nearly any operation (add, sub, mul, div, etc)

That's what we do for hw intrinsics, presently.

@TIHan
Copy link
Contributor Author

TIHan commented Dec 7, 2022

Why just and/or/xor and not more generally?

At the moment, we only change and/or/xor operator's type to a small type in a specific case. I don't know of any cases where the operators(add/sub/mul/etc) could be small types.

@TIHan TIHan merged commit b858484 into dotnet:main Dec 7, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jan 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@BruceForstall BruceForstall BruceForstall approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@TIHan TIHan

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.

3 participants

@TIHan @tannergooding @BruceForstall