Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
[TRI] Remove reserved registers in getRegPressureSetLimit #118787
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
base: users/wangpc-pp/spr/main.tri-remove-reserved-registers-in-getregpressuresetlimit
Are you sure you want to change the base?
[TRI] Remove reserved registers in getRegPressureSetLimit #118787
Changes from 1 commit
2ec06c7
fae615d
723597a
5adec35
9737ec8
538b48d
f5de3e9
47c3275
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @kasuga-fj! What do you think about this part? I just removed the code below as it seems to be unnecessary now. Related patch: #74807
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I think just removing
fixed
registers is not enough here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for caring. I dared to replace it from
RegisterClassInfo::getRegPressureSetLimit
before (in #87312). This is because there were duplicate registers between whatRegisterClassInfo::getRegPressureSetLimit
removes and thefixed
registers mentioned here. IfRegisterClassInfo::getRegPressureSetLimit
now takes care of both, I don't think it's a problem to remove this part as you did.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reason we drop the call to
compute
from computePSetLimit here? It looks like we're doing this instead:It looks like this is related to what is in
compute
, but we seem to drop some costing logic.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason why we need a
compute
is because we need to compute the number of allocatable registers and then get the number of reserved registers (but actually I think we don't need to callcompute
explicitly asgetNumAllocatableRegs
is cached, it will callcompute
inget(RC)
):The logic of calculating
NumRegs
incompute
is:RCI.NumRegs
is the total number of registers minus the number of reserved registers.And in this patch, we change it to calculate the number of reserved registers first and calculate the allocatable registers later. The effect should be the same I think.