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

Prepare for addition of disjoint flag from upstream llvm #2851

Merged
merged 2 commits into from
Nov 28, 2023

Conversation

dstutt
Copy link
Member

@dstutt dstutt commented Nov 28, 2023

Upstream llvm is introducing a new disjoint flag that makes it easier to determine when an or instruction was originally an add - which can be useful in some optimisations.

Since neither of the affected tests were auto-generated, I've added something that allows for the disjoint flag to be optionally present.

@dstutt dstutt requested a review from a team as a code owner November 28, 2023 09:52
@jasilvanus
Copy link
Contributor

jasilvanus commented Nov 28, 2023

There seems to be lots of diff caused by replacing CRLF by Unix line endings -- could you please factor that out into a separate commit?

Edit: I just realized that this is what you meant with "un-dosifying" -- I thought "dosify" was just a rare verb that I could not make sense of :)

@dstutt
Copy link
Member Author

dstutt commented Nov 28, 2023

There seems to be lots of diff caused by replacing CRLF by Unix line endings -- could you please factor that out into a separate commit?

Edit: I just realized that this is what you meant with "un-dosifying" -- I thought "dosify" was just a rare verb that I could not make sense of :)

I could make it a separate commit if that's preferable. It just makes the porting process a bit more of a hassle. But I do agree that it adds unnecessary mess.

@jasilvanus
Copy link
Contributor

There seems to be lots of diff caused by replacing CRLF by Unix line endings -- could you please factor that out into a separate commit?
Edit: I just realized that this is what you meant with "un-dosifying" -- I thought "dosify" was just a rare verb that I could not make sense of :)

I could make it a separate commit if that's preferable. It just makes the porting process a bit more of a hassle. But I do agree that it adds unnecessary mess.

Either is fine with me if it really helps the porting process.
In general I think separating out noise is good practice though.

Upstream llvm is introducing a new disjoint flag that makes it easier to
determine when an or instruction was originally an add - which can be useful in
some optimisations.

Since neither of the affected tests were auto-generated, I've added something
that allows for the disjoint flag to be optionally present.
@dstutt
Copy link
Member Author

dstutt commented Nov 28, 2023

I made it a separate commit.

Copy link
Contributor

@jasilvanus jasilvanus left a comment

Choose a reason for hiding this comment

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

Thanks!

(Would squashing on merge help the porting issue?)

@dstutt
Copy link
Member Author

dstutt commented Nov 28, 2023

Thanks!

(Would squashing on merge help the porting issue?)

I'm going to port it as 2 commits - I think it makes sense for the same reason as for here (keep the noise as a separate commit).

@amdvlk-admin
Copy link

Test summary for commit 1217b26

CTS tests (Failed: 0/138443)
  • Built with version 1.3.5.2
  • Ubuntu navi3x, Srdcvk
    • Passed: 35211/69228 (50.9%)
    • Failed: 0/69228 (0.0%)
    • Not Supported: 34017/69228 (49.1%)
    • Warnings: 0/69228 (0.0%)
    Ubuntu navi2x, Srdcvk
    • Passed: 35242/69215 (50.9%)
    • Failed: 0/69215 (0.0%)
    • Not Supported: 33973/69215 (49.1%)
    • Warnings: 0/69215 (0.0%)

@amdvlk-admin
Copy link

Test summary for commit ad2b7bd

CTS tests (Failed: 0/138378)
  • Built with version 1.3.5.2
  • Ubuntu navi3x, Srdcvk
    • Passed: 35162/69163 (50.8%)
    • Failed: 0/69163 (0.0%)
    • Not Supported: 34001/69163 (49.2%)
    • Warnings: 0/69163 (0.0%)
    Ubuntu navi2x, Srdcvk
    • Passed: 35242/69215 (50.9%)
    • Failed: 0/69215 (0.0%)
    • Not Supported: 33973/69215 (49.1%)
    • Warnings: 0/69215 (0.0%)

@dstutt dstutt merged commit cb96c50 into GPUOpen-Drivers:dev Nov 28, 2023
10 checks passed
@dstutt dstutt deleted the handle-disjoint branch November 28, 2023 10:29
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.

4 participants