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

[mlir][spirv] Migrate mlir-vulkan-runner to follow other client API runners #73457

Open
antiagainst opened this issue Nov 26, 2023 · 11 comments
Open
Assignees
Labels
good first issue https://github.com/llvm/llvm-project/contribute mlir:spirv

Comments

@antiagainst
Copy link
Member

We added mlir-vulkan-runner in way early days of MLIR. Recently various MLIR client API runners (e.g., mlir-cuda-runner) were removed in favor of performing translation using mlir-opt and then leverage mlir-cpu-runner as the host coordnation mechanism. See @joker-eph's #65539 (comment) for more context. We should migrate mlir-vulkan-runner to follow there. This would unify the runner story in MLIR to have one single mlir-runner, as @Jianhui-Li's #65539 (comment) here.

@llvmbot
Copy link
Collaborator

llvmbot commented Nov 26, 2023

@llvm/issue-subscribers-mlir-spirv

Author: Lei Zhang (antiagainst)

We added mlir-vulkan-runner in way early days of MLIR. Recently various MLIR client API runners (e.g., mlir-cuda-runner) were removed in favor of performing translation using `mlir-opt` and then leverage `mlir-cpu-runner` as the host coordnation mechanism. See @joker-eph's https://github.com//pull/65539#issuecomment-1710872236 for more context. We should migrate mlir-vulkan-runner to follow there. This would unify the runner story in MLIR to have one single mlir-runner, as @Jianhui-Li's https://github.com//pull/65539#issuecomment-1712414848 here.

@antiagainst antiagainst added the good first issue https://github.com/llvm/llvm-project/contribute label Nov 26, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented Nov 26, 2023

Hi!

This issue may be a good introductory issue for people new to working on LLVM. If you would like to work on this issue, your first steps are:

  1. In the comments of the issue, request for it to be assigned to you.
  2. Fix the issue locally.
  3. Run the test suite locally.
    3.1) Remember that the subdirectories under test/ create fine-grained testing targets, so you can
    e.g. use make check-clang-ast to only run Clang's AST tests.
  4. Create a Git commit.
  5. Run git clang-format HEAD~1 to format your changes.
  6. Open a pull request to the upstream repository on GitHub.
    6.1) Detailed instructions can be found here.

If you have any further questions about this issue, don't hesitate to ask via a comment on this Github issue.

@llvmbot
Copy link
Collaborator

llvmbot commented Nov 26, 2023

@llvm/issue-subscribers-good-first-issue

Author: Lei Zhang (antiagainst)

We added mlir-vulkan-runner in way early days of MLIR. Recently various MLIR client API runners (e.g., mlir-cuda-runner) were removed in favor of performing translation using `mlir-opt` and then leverage `mlir-cpu-runner` as the host coordnation mechanism. See @joker-eph's https://github.com//pull/65539#issuecomment-1710872236 for more context. We should migrate mlir-vulkan-runner to follow there. This would unify the runner story in MLIR to have one single mlir-runner, as @Jianhui-Li's https://github.com//pull/65539#issuecomment-1712414848 here.

@bhaskar1001101
Copy link

Hi. I would like to work on this. I'll try to take ⚙ D98396 [mlir] Remove mlir-cuda-runner as reference.

@antiagainst
Copy link
Member Author

Hi @bhaskar1001101, sorry I missed your reply previously. Are you still interested to push this forward? If so I'll assign you to the issue. :)

@Sh0g0-1758
Copy link
Contributor

Hello, I am new to LLVM and would like to work on this. @antiagainst , can you please assign me this issue.

@rengolin
Copy link
Member

rengolin commented Jan 9, 2024

@bhaskar1001101 and @Sh0g0-1758, you both have shown interest, so I assigned both of you. Can you work together on this?

To be clear, the idea is to remove mlir-vulkan-runner, moving the logic inside mlir-cpu-runner (like CUDA did) and then renaming mlir-cpu-runner to just mlir-runner.

@antiagainst
Copy link
Member Author

Yeah. Note that I've marked this as good first issue but it's a relative large effort than normal, and may need some reading and understanding of mlir runners and vulkan specficially. Please let me know if you have questions. There are also other smaller good first issues if you are interested, just search with label "mlir:spirv" and "good first issues" to find them.

@Sh0g0-1758
Copy link
Contributor

yes sure thing @antiagainst . I was getting familiar with mlir and will update you when a question of which I can't answer on the discourse arise.

@antiagainst
Copy link
Member Author

Hey @bhaskar1001101 and @Sh0g0-1758, is this something you are still interested? Have you able to make progress on it?

@Rajveer100
Copy link
Contributor

@antiagainst
Any particular insights that you would like to give apart from the comment links in the issue description?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue https://github.com/llvm/llvm-project/contribute mlir:spirv
Projects
None yet
Development

No branches or pull requests

6 participants