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.

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

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

Merged
merged 1 commit into from
Dec 7, 2022

Conversation

TIHan
Copy link
Contributor

@TIHan TIHan commented Dec 2, 2022

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

@dotnet-issue-labeler dotnet-issue-labeler bot 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: -

@@ -447,6 +444,30 @@ class Lowering final : public Phase
return m_lsra->isContainableMemoryOp(node);
}

// 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

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
Copy link
Contributor Author

TIHan commented Dec 2, 2022

@dotnet/jit-contrib ready for review

@tannergooding
Copy link
Member

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.
Labels
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.

3 participants