Skip to content

[AMDGPU] [SIFrameLowering] Use LiveRegUnits instead of LivePhysRegs #65962

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
merged 2 commits into from
Sep 21, 2023
Merged

[AMDGPU] [SIFrameLowering] Use LiveRegUnits instead of LivePhysRegs #65962

merged 2 commits into from
Sep 21, 2023

Conversation

@prtaneja
Copy link
Contributor

@prtaneja prtaneja commented Sep 11, 2023

Enhanced liveness checks in LiveRegUnits and then used the modified LiveRegUnits to track liveness in SIFrameLowering.
The first commit is the enhancement to LiveRegUnits and the second commit changes the liveness tracking utility to LiveRegUnits in SIFrameLowering.

@prtaneja prtaneja requested review from a team as code owners September 11, 2023 13:26
@prtaneja prtaneja requested review from cdevadas and removed request for a team September 11, 2023 13:30
jayfoad
jayfoad reviewed Sep 11, 2023
View reviewed changes
Copy link
Contributor

@jayfoad jayfoad left a comment

Choose a reason for hiding this comment

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

I don't really like the naming of "contains" vs "available" but I don't have a better suggestion so I guess it is OK.

jayfoad
jayfoad approved these changes Sep 11, 2023
View reviewed changes
Copy link
Contributor

@jayfoad jayfoad left a comment

Choose a reason for hiding this comment

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

Whole stack LGTM. Thanks for working on this! Have you measured any compile-time difference from this change?

@arsenm
Copy link
Contributor

arsenm commented Sep 11, 2023

FYI you're currently only supposed to push one commit per pull request

@prtaneja
Copy link
Contributor Author

prtaneja commented Sep 11, 2023

FYI you're currently only supposed to push one commit per pull request
I did want to follow the suggested "one commit per PR" as prescribed in the policy but these commits are actually two separate related changes and since stacked reviews are not yet supported here, creating a monolithic review for such a code change didn't seem ideal.
Would you have any suggestions on dealing with this currently?

@arsenm
Copy link
Contributor

arsenm commented Sep 11, 2023

FYI you're currently only supposed to push one commit per pull request
I did want to follow the suggested "one commit per PR" as prescribed in the policy but these commits are actually two separate related changes and since stacked reviews are not yet supported here, creating a monolithic review for such a code change didn't seem ideal.
Would you have any suggestions on dealing with this currently?

We don't really have a solution right now

@cdevadas
Copy link
Collaborator

cdevadas commented Sep 12, 2023

Why not split them into two separate PRs and mention the generic patch's PR info in the second one (AMDGPU-specific)?

@prtaneja prtaneja mentioned this pull request Sep 12, 2023
Closed
@prtaneja
Copy link
Contributor Author

prtaneja commented Sep 13, 2023

Whole stack LGTM. Thanks for working on this! Have you measured any compile-time difference from this change?

compileTimeDiff.log

Here's the difference between compile time from both the original and modified builds on compiling tests from llvm-test-suite

@arsenm
Copy link
Contributor

arsenm commented Sep 13, 2023

Whole stack LGTM. Thanks for working on this! Have you measured any compile-time difference from this change?

compileTimeDiff.log

Here's the difference between compile time from both the original and modified builds on compiling tests from llvm-test-suite

So didn't really do anything for x86, which is expected. Ideally we would have GPU compute compile time tracking but we don't

@prtaneja prtaneja force-pushed the SIFrameLoweringLiveness branch from da706a7 to 4624fea Compare September 14, 2023 09:55
@prtaneja
Copy link
Contributor Author

prtaneja commented Sep 14, 2023 •
edited
Loading

Have updated this branch to retain 'available' in LiveRegUnits as it is while still updating SIFrameLowering (as suggested by @arsenm in [LiveRegUnits] Enhanced the register liveness check)

@prtaneja prtaneja force-pushed the SIFrameLoweringLiveness branch from 4624fea to cd00501 Compare September 14, 2023 11:06
@prtaneja prtaneja force-pushed the SIFrameLoweringLiveness branch from cd00501 to a69dec0 Compare September 15, 2023 05:16
@prtaneja
Copy link
Contributor Author

prtaneja commented Sep 15, 2023

Whole stack LGTM. Thanks for working on this! Have you measured any compile-time difference from this change?

compileTimeDiff.log
Here's the difference between compile time from both the original and modified builds on compiling tests from llvm-test-suite

So didn't really do anything for x86, which is expected. Ideally we would have GPU compute compile time tracking but we don't

If it serves as a proxy for AMDGPU compilation time, here's the difference in time it takes to complete testing the lit tests in "llvm/test/CodeGen/AMDGPU", with the modified branch with LiveRegUnits taking about a second less in multiple trials.
litTestingTime.patch

@jayfoad
Copy link
Contributor

jayfoad commented Sep 15, 2023

with the modified branch with LiveRegUnits taking about a second less in multiple trials

Sounds good!

@prtaneja prtaneja force-pushed the SIFrameLoweringLiveness branch from 212ecfe to 8ea9e74 Compare September 19, 2023 05:51
@Pranav-Taneja
[SIFrameLowering] Added TODO to findUnusedRegister
5ee8e4f 
We need an 'isRegUnitUsed' function similar to
'isPhysRegUsed' in order to implement the loop
in 'findUnusedRegister' in terms of MCRegUnits.
@prtaneja prtaneja force-pushed the SIFrameLoweringLiveness branch from 8ea9e74 to 5ee8e4f Compare September 19, 2023 05:55
cdevadas
cdevadas approved these changes Sep 20, 2023
View reviewed changes
Copy link
Collaborator

@cdevadas cdevadas left a comment

Choose a reason for hiding this comment

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

LGTM.

@prtaneja prtaneja merged commit 6d9b963 into llvm:main Sep 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jayfoad jayfoad jayfoad approved these changes

@cdevadas cdevadas cdevadas approved these changes

Assignees

No one assigned

Labels

backend:AArch64 backend:AMDGPU backend:ARM backend:RISC-V llvm:regalloc

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@prtaneja @arsenm @cdevadas @jayfoad @llvmbot @Pranav-Taneja