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.

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

Update shader tests after LLVM update #2835

Merged
merged 1 commit into from
Nov 22, 2023
Merged

Update shader tests after LLVM update #2835

merged 1 commit into from
Nov 22, 2023

Conversation

petar-avramovic
Copy link
Contributor

Update shader tests after a set of upstream changes in inst-combine. Last patch in the set is 5271d330770573d2df772aa0fa50d958f44400aa llvm/llvm-project#66787
Disable affected tests in this patch.
TODO: re-enable tests once everything has propagated.

@petar-avramovic
Update shader tests after LLVM update
530aa41 
Update shader tests after a set of upstream changes in inst-combine.
Last patch in the set is 5271d330770573d2df772aa0fa50d958f44400aa
llvm/llvm-project#66787
Disable affected tests in this patch.
TODO: re-enable tests once everything has propagated.
@petar-avramovic petar-avramovic requested a review from a team as a code owner November 22, 2023 14:17
@jasilvanus
Copy link
Contributor

jasilvanus commented Nov 22, 2023 •
edited
Loading

Not specific to this PR, but maybe we should improve on how we keep track of disabled tests, e.g. by requiring to open an issue?

This is a git blame on currently disabled tests, some of these commits are quite old and mention to "soon enable tests again" once the update has propagated:

298e95edad (wenqinli 2023-05-19 14:15:25 +0800 16) ; REQUIRES: do-not-run-me
298e95edad (wenqinli 2023-05-19 14:15:25 +0800 20) ; REQUIRES: do-not-run-me
298e95edad (wenqinli 2023-05-19 14:15:25 +0800 3) // REQUIRES: do-not-run-me
298e95edad (wenqinli 2023-05-19 14:15:25 +0800 3) ; REQUIRES: do-not-run-me
4d8954d9ec (Chen Huang 2023-10-24 15:27:13 +0800 2) ; REQUIRES: do-not-run-me
4d8954d9ec (Chen Huang 2023-10-24 15:27:13 +0800 5) ; REQUIRES: do-not-run-me
4f055cce2f (Jay Foad 2023-07-07 13:58:56 +0100 3) ; REQUIRES: do-not-run-me
5db5dabbef (petar-avramovic 2023-10-13 09:23:57 +0200 4) ; REQUIRES: do-not-run-me
5db5dabbef (petar-avramovic 2023-10-13 09:23:57 +0200 5) ; REQUIRES: do-not-run-me
5db5dabbef (petar-avramovic 2023-10-13 09:23:57 +0200 5) ; REQUIRES: do-not-run-me
5db5dabbef (petar-avramovic 2023-10-13 09:23:57 +0200 6) ; REQUIRES: do-not-run-me
5db5dabbef (petar-avramovic 2023-10-13 09:23:57 +0200 6) ; REQUIRES: do-not-run-me
82ef9665ff (Mirko Brkušanin 2023-11-09 15:32:09 +0100 6) ; REQUIRES: do-not-run-me
8471e46f3e (Mirko Brkušanin 2023-08-31 12:22:58 +0200 17) ; REQUIRES: do-not-run-me
8471e46f3e (Mirko Brkušanin 2023-08-31 12:22:58 +0200 17) ; REQUIRES: do-not-run-me
854b7b8ab1 (Mirko Brkušanin 2023-07-20 12:35:53 +0200 4) ; REQUIRES: do-not-run-me
854b7b8ab1 (Mirko Brkušanin 2023-07-20 12:35:53 +0200 4) ; REQUIRES: do-not-run-me
854b7b8ab1 (Mirko Brkušanin 2023-07-20 12:35:53 +0200 4) ; REQUIRES: do-not-run-me
ce2c12c412 (wenqinli 2023-08-07 14:13:20 +0800 3) ; REQUIRES: do-not-run-me
ce2c12c412 (wenqinli 2023-08-07 14:13:20 +0800 6) ; REQUIRES: do-not-run-me
ce2c12c412 (wenqinli 2023-08-07 14:13:20 +0800 9) ; REQUIRES: do-not-run-me
dd36054e65 (petar-avramovic 2023-10-13 14:32:49 +0200 5) ; REQUIRES: do-not-run-me
e51543dc52 (chuang13 2023-11-20 17:47:19 +0800 3) // REQUIRES: do-not-run-me
ffc49b2a07 (wenqinli 2023-01-30 16:52:11 +0800 5) ; REQUIRES: do-not-run-me

Produced by:

for item in $(rg "do-not-run-me" -n  | cut -d ' ' -f1 ); do file="$(echo $item | cut -d ':' -f1)"; line="$(echo $item| cut -d ':' -f2)"; git blame -L${line},${line} $file; done | sort

@amdvlk-admin
Copy link

Test summary for commit 530aa41

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%)

@Flakebi
Copy link
Member

Flakebi commented Nov 22, 2023

Not specific to this PR, but maybe we should improve on how we keep track of disabled tests, e.g. by requiring to open an issue?

FWIW, we’d like to simply ; REQUIRE: LLVM_MAIN_REVISION >= xyz, but we need https://reviews.llvm.org/D154987 for that (David started something simpler, but it resulted in something complex and powerful).

dstutt
dstutt approved these changes Nov 22, 2023
View reviewed changes
Copy link
Member

@dstutt dstutt left a comment

Choose a reason for hiding this comment

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

LGTM too

@dstutt
Copy link
Member

dstutt commented Nov 22, 2023

Not specific to this PR, but maybe we should improve on how we keep track of disabled tests, e.g. by requiring to open an issue?

This is a git blame on currently disabled tests, some of these commits are quite old and mention to "soon enable tests again" once the update has propagated:

298e95edad (wenqinli 2023-05-19 14:15:25 +0800 16) ; REQUIRES: do-not-run-me
298e95edad (wenqinli 2023-05-19 14:15:25 +0800 20) ; REQUIRES: do-not-run-me
298e95edad (wenqinli 2023-05-19 14:15:25 +0800 3) // REQUIRES: do-not-run-me
298e95edad (wenqinli 2023-05-19 14:15:25 +0800 3) ; REQUIRES: do-not-run-me
4d8954d9ec (Chen Huang 2023-10-24 15:27:13 +0800 2) ; REQUIRES: do-not-run-me
4d8954d9ec (Chen Huang 2023-10-24 15:27:13 +0800 5) ; REQUIRES: do-not-run-me
4f055cce2f (Jay Foad 2023-07-07 13:58:56 +0100 3) ; REQUIRES: do-not-run-me
5db5dabbef (petar-avramovic 2023-10-13 09:23:57 +0200 4) ; REQUIRES: do-not-run-me
5db5dabbef (petar-avramovic 2023-10-13 09:23:57 +0200 5) ; REQUIRES: do-not-run-me
5db5dabbef (petar-avramovic 2023-10-13 09:23:57 +0200 5) ; REQUIRES: do-not-run-me
5db5dabbef (petar-avramovic 2023-10-13 09:23:57 +0200 6) ; REQUIRES: do-not-run-me
5db5dabbef (petar-avramovic 2023-10-13 09:23:57 +0200 6) ; REQUIRES: do-not-run-me
82ef9665ff (Mirko Brkušanin 2023-11-09 15:32:09 +0100 6) ; REQUIRES: do-not-run-me
8471e46f3e (Mirko Brkušanin 2023-08-31 12:22:58 +0200 17) ; REQUIRES: do-not-run-me
8471e46f3e (Mirko Brkušanin 2023-08-31 12:22:58 +0200 17) ; REQUIRES: do-not-run-me
854b7b8ab1 (Mirko Brkušanin 2023-07-20 12:35:53 +0200 4) ; REQUIRES: do-not-run-me
854b7b8ab1 (Mirko Brkušanin 2023-07-20 12:35:53 +0200 4) ; REQUIRES: do-not-run-me
854b7b8ab1 (Mirko Brkušanin 2023-07-20 12:35:53 +0200 4) ; REQUIRES: do-not-run-me
ce2c12c412 (wenqinli 2023-08-07 14:13:20 +0800 3) ; REQUIRES: do-not-run-me
ce2c12c412 (wenqinli 2023-08-07 14:13:20 +0800 6) ; REQUIRES: do-not-run-me
ce2c12c412 (wenqinli 2023-08-07 14:13:20 +0800 9) ; REQUIRES: do-not-run-me
dd36054e65 (petar-avramovic 2023-10-13 14:32:49 +0200 5) ; REQUIRES: do-not-run-me
e51543dc52 (chuang13 2023-11-20 17:47:19 +0800 3) // REQUIRES: do-not-run-me
ffc49b2a07 (wenqinli 2023-01-30 16:52:11 +0800 5) ; REQUIRES: do-not-run-me

Produced by:

for item in $(rg "do-not-run-me" -n  | cut -d ' ' -f1 ); do file="$(echo $item | cut -d ':' -f1)"; line="$(echo $item| cut -d ':' -f2)"; git blame -L${line},${line} $file; done | sort

@jasilvanus Yes, I think that's a good idea. Would you mind doing that?
Maybe one issue with these in - and then protocol should be a new issue for each time we have to do this in future?

@trenouf trenouf merged commit 0b15602 into GPUOpen-Drivers:dev Nov 22, 2023
9 checks passed
@jasilvanus
Copy link
Contributor

@jasilvanus Yes, I think that's a good idea. Would you mind doing that? Maybe one issue with these in - and then protocol should be a new issue for each time we have to do this in future?

I'd rather have one GitHub issue per commit above assigned to the authors, so we can make progress in re-enabling tests and closing issues. I can create the initial ones if others feel positive about this.

@dstutt
Copy link
Member

dstutt commented Nov 22, 2023

I wonder if it makes sense for one person to try fixing all of them in one go - and then filing issues for any that still fail?

@dstutt
Copy link
Member

dstutt commented Nov 23, 2023

I tried re-enabling the tests. Some now work. See #2838

This was referenced Nov 24, 2023
Closed
Closed
Closed
Closed
Closed
Closed
Closed
@jasilvanus
Copy link
Contributor

I have opened issues for the remaining tests, see the referenced issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dstutt dstutt dstutt approved these changes

@Flakebi Flakebi Flakebi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@petar-avramovic @jasilvanus @amdvlk-admin @Flakebi @dstutt @trenouf