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

[Hexagon] Use LiveRegUnits #84112

Merged
merged 1 commit into from
Mar 8, 2024
Merged

[Hexagon] Use LiveRegUnits #84112

merged 1 commit into from
Mar 8, 2024

Conversation

AtariDreams
Copy link
Contributor

No description provided.

@AtariDreams AtariDreams closed this Mar 6, 2024
@AtariDreams AtariDreams deleted the hexagon branch March 6, 2024 04:17
@AtariDreams AtariDreams restored the hexagon branch March 6, 2024 04:18
@AtariDreams AtariDreams reopened this Mar 6, 2024
@AtariDreams AtariDreams marked this pull request as draft March 6, 2024 04:18
@kparzysz
Copy link
Contributor

kparzysz commented Mar 6, 2024

LGTM. You can remove the IsLive definition, as it's not used anymore. Thanks.

@AtariDreams AtariDreams marked this pull request as ready for review March 6, 2024 16:10
@AtariDreams
Copy link
Contributor Author

LGTM. You can remove the IsLive definition, as it's not used anymore. Thanks.

Good now? Also I do not have commit permissions.

@AtariDreams
Copy link
Contributor Author

LGTM. You can remove the IsLive definition, as it's not used anymore. Thanks.

I cannot do that because tests break and report bad machine code.

@kparzysz
Copy link
Contributor

kparzysz commented Mar 6, 2024

Please stop using force-pushes. They can mess up previous review comments.

@kparzysz
Copy link
Contributor

kparzysz commented Mar 6, 2024

There was a mistake in my comment: IsLive is equivalent to !LPR.available. Please restore the previous change and apply this correction.

@AtariDreams
Copy link
Contributor Author

There was a mistake in my comment: IsLive is equivalent to !LPR.available. Please restore the previous change and apply this correction.

Done!

Copy link
Contributor

@kparzysz kparzysz left a comment

Choose a reason for hiding this comment

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

Please remove IsLive, LGTM with that change.

@AtariDreams
Copy link
Contributor Author

Please remove IsLive, LGTM with that change.

I tried but then tests started failing, so I cannot do that.

@kparzysz
Copy link
Contributor

kparzysz commented Mar 7, 2024

I tried but then tests started failing, so I cannot do that.

The IsLive function checks if the register is in use (i.e. live). We then use it to set the kill flag on it: if the register is not live, then the use of it that we see (we're going backwards here) is the last one, so it's a kill. The available function in LiveRegUnits does exactly what IsLive is doing here. In this context "available" means "not in use", i.e. not live.

Is you replace the use of IsLive with !available it should work.

@AtariDreams
Copy link
Contributor Author

I tried but then tests started failing, so I cannot do that.

The IsLive function checks if the register is in use (i.e. live). We then use it to set the kill flag on it: if the register is not live, then the use of it that we see (we're going backwards here) is the last one, so it's a kill. The available function in LiveRegUnits does exactly what IsLive is doing here. In this context "available" means "not in use", i.e. not live.

Is you replace the use of IsLive with !available it should work.

Done! @kparzysz

Copy link
Contributor

@kparzysz kparzysz left a comment

Choose a reason for hiding this comment

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

LGTM

@kparzysz kparzysz merged commit d2c49f1 into llvm:main Mar 8, 2024
4 checks passed
@AtariDreams AtariDreams deleted the hexagon branch March 8, 2024 15:44
miguelraz pushed a commit to miguelraz/llvm-project that referenced this pull request Mar 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants