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

Handle more than 64 registers - Part 5 #103188

Merged
merged 21 commits into from
Jun 11, 2024
Merged

Conversation

kunalspathak
Copy link
Member

  • Convert all the RBM_REG* to regMaskTP
  • Introduce SRBM_REG* that can be used at places where we just need SingleTypeRegSet
  • Start using regMaskTP for registers at kill RefPosition

@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 Jun 8, 2024
Copy link
Contributor

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

@kunalspathak kunalspathak marked this pull request as ready for review June 10, 2024 15:48
@kunalspathak kunalspathak requested a review from jakobbotsch June 10, 2024 15:48
@kunalspathak
Copy link
Member Author

@dotnet/jit-contrib @jakobbotsch PTAL

@@ -9851,7 +9851,8 @@ class Compiler
// On these platforms we assume the register that the target is
// passed in is preserved by the validator and take care to get the
// target from the register for the call (even in debug mode).
static_assert_no_msg((RBM_VALIDATE_INDIRECT_CALL_TRASH & (1 << REG_VALIDATE_INDIRECT_CALL_ADDR)) == 0);
static_assert_no_msg((RBM_VALIDATE_INDIRECT_CALL_TRASH & regMaskTP(1 << REG_VALIDATE_INDIRECT_CALL_ADDR)) ==
Copy link
Member

Choose a reason for hiding this comment

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

We can make IsRegNumInMask constexpr to make this a bit more natural (feel free to do as part of a follow-up).

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried doing that, but as a cascade effect I need to make genSingleTypeRegSet, getRegForType(), etc. as constexpr which I am not sure is worth doing for just an assert. Will pass this on for now.

Comment on lines +6870 to +6872
hasPC = (imm & SRBM_PC) != 0;
hasLR = (imm & SRBM_LR) != 0;
imm &= ~(SRBM_PC | SRBM_LR);
Copy link
Member

Choose a reason for hiding this comment

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

I guess this means there is (and was) an invariant on the exact indices defined for these registers?

Copy link
Member Author

Choose a reason for hiding this comment

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

probably.

Comment on lines +267 to +268
static constexpr regMaskTP CreateFromRegNum(regNumber reg, regMaskSmall mask)
{
Copy link
Member

Choose a reason for hiding this comment

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

Is mask needed as a parameter? Can't we compute it here as 1 << reg?

Copy link
Member Author

Choose a reason for hiding this comment

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

predicate registers starts from 64 thru 80 and the mask of them start with 0x0. I can make it work, but just want to rely on the mask that we already have in register files and how we use them currently.

Copy link
Member

Choose a reason for hiding this comment

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

For predicate registers it would just be (1 << (reg - 64)) under the if, I suppose.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I don't have much preference on which way or the other, but will keep this in mind during follow up.

Copy link
Member

@jakobbotsch jakobbotsch left a comment

Choose a reason for hiding this comment

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

Nice, this looks great to me. Most changes look mechanical.

@kunalspathak
Copy link
Member Author

/rerun

@kunalspathak
Copy link
Member Author

@dotnet-policy-service rerun

1 similar comment
@kunalspathak
Copy link
Member Author

@dotnet-policy-service rerun

@kunalspathak kunalspathak merged commit d06fb08 into dotnet:main Jun 11, 2024
105 of 107 checks passed
@kunalspathak kunalspathak deleted the part5 branch June 11, 2024 17:18
@github-actions github-actions bot locked and limited conversation to collaborators Jul 12, 2024
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.

2 participants