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

LSRA: Make genRegMask() return regMaskTP #102783

Merged
merged 12 commits into from
May 29, 2024
Merged

Conversation

kunalspathak
Copy link
Member

@kunalspathak kunalspathak commented May 28, 2024

Address feedback in #102592 (comment)

  • Changed back genRegMask() from SingleTypeRegSet to regMaskTP
  • Changed back rbmAllFloat from SingleTypeRegSet to regMaskTP
  • Added method genSingleTypeRegMask() that will be used in LSRA whenever we want to map a register number to the mask
  • genRegMask() will continue to return regMaskTP that will be used in non-LSRA code base.

@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 May 28, 2024
Copy link
Contributor

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

@kunalspathak
Copy link
Member Author

@shushanhf @dotnet/samsung

@kunalspathak
Copy link
Member Author

@dotnet/jit-contrib @jakobbotsch PTAL

@kunalspathak kunalspathak requested a review from jakobbotsch May 29, 2024 04:26
Comment on lines +848 to +849
// TODO: Populate regMaskTP based on reg
return genSingleTypeRegMask(reg);
Copy link
Member

Choose a reason for hiding this comment

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

Even with predicate registers I would expect us to end up with basically the same logic as before, i.e. something like regMaskTP(1) << reg, probably encapsulating that operation inside regMaskTP. That is, we leave the regNumber mappings for predicate registers to just follow the float registers so that all reg numbers are unique.

Of course LSRA is going to need slightly different logic to map SingleTypeRegSet to regMaskTP which ends up requiring additional context about the type of set.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, we basically will use the old logic of genRegMask() and use that value to set appropriate field of regMaskTP.

Comment on lines +451 to +455
#ifdef TARGET_AMD64
return RBM_FLT_CALLEE_TRASH.GetFloatRegSet();
#else
return RBM_FLT_CALLEE_TRASH;
#endif // TARGET_AMD64
Copy link
Member

Choose a reason for hiding this comment

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

Is the ifdef necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, for amd64, RBM_FLT_CALLEE_TRASH maps to get_RBM_FLT_CALLEE_TRASH() which returns regMaskTP but for other platforms, it is just uint64_t or SingleTypeRegSet.

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 will see if I can unify this in future as this work evolves.

Comment on lines +860 to 865
#ifdef TARGET_AMD64
availableFloatRegs = RBM_ALLFLOAT.GetFloatRegSet();
availableDoubleRegs = RBM_ALLDOUBLE.GetFloatRegSet();
#else
availableFloatRegs = RBM_ALLFLOAT;
availableDoubleRegs = RBM_ALLDOUBLE;
Copy link
Member

Choose a reason for hiding this comment

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

Same question, can this use the AMD64 logic for everyone?

Comment on lines +8804 to +8808
#ifdef TARGET_XARCH
freeRegs &= RBM_CALLEE_TRASH.GetRegSetForType(type);
#else
freeRegs &= RBM_CALLEE_TRASH;
#endif
Copy link
Member

Choose a reason for hiding this comment

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

Ditto

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.

This LGTM. A few minor questions/general comments on future design.

@kunalspathak
Copy link
Member Author

/ba-g failures are dotnet/dnceng#2378 and #102773

@kunalspathak kunalspathak merged commit 23e15a5 into dotnet:main May 29, 2024
100 of 107 checks passed
Ruihan-Yin pushed a commit to Ruihan-Yin/runtime that referenced this pull request May 30, 2024
* genRegMask

* Make genRegMask() return regMaskTP, introduce genSingleTypeRegMask() for LSRA

* Make allFloat,allMask regMaskTP instead of `SingleTypeRegSet` so no affect on non-LSRA code

* jit format

* fix build errors

* fix loongarch and risc

* fix a typo

* move more `genSingleTypeRegMask()`

* jit format

* Make genRegMask() use genSingleType*()

* fix build errors

* jit format
@github-actions github-actions bot locked and limited conversation to collaborators Jun 29, 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