Skip to content

[GitHub] Add main LLVM Dialect path to MLIR LLVM PR subscribers #65513

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

Merged
merged 2 commits into from
Sep 7, 2023

Conversation

zero9178
Copy link
Member

@zero9178 zero9178 commented Sep 6, 2023

The LLVM Dialect in MLIR, which the mlir-llvm team is supposed to provide notifications for, is 98% not nested in a directory called LLVM but rather LLVMIR. The former only contains some tests.

This should make PRs such as #65508 add the team as codeowner.

The LLVM Dialect in MLIR, which the `mlir-llvm` team is supposed to provide notifications for, is 98% not nested in a directory called LLVM but rather LLVMIR. The former only contains some tests.

This should make PRs such as llvm#65508 add the team as codeowner.
@@ -120,6 +120,7 @@

# mlir-llvm
/mlir/**/LLVM/ @llvm/pr-subscribers-mlir-llvm
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shall we just remove the trailing / here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just tested this using git check-ignore (since this should be the same syntax as for the codeowners file apparently) and that at least did not work for any checks in the LLVMIR directory.
Changing it to either LLVMIR with or without the slash works, but it seems to me like it doesn't treat trailing positions specially.

Unless GitHub CodeOwners files do it differently.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK! We need a trailing *, which means most of the existing patterns are incorrect...
mlir/**/LLVM* should match both here.

Copy link
Collaborator

@joker-eph joker-eph left a comment

Choose a reason for hiding this comment

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

Thanks!

We're moving away from CODEOWNERS for subscription to replace it with a labeler solution, but feel free to land this though

@zero9178 zero9178 merged commit 38c5d23 into llvm:main Sep 7, 2023
@zero9178 zero9178 deleted the fix-mlir-llvmir-codeowners branch September 8, 2023 11:53
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.

2 participants