llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
27.61k stars 11.35k forks source link

[mlir][spirv] Support abbreviated SPIR-V type format #58132

Open kuhar opened 1 year ago

kuhar commented 1 year ago

We can elide the leading dialect prefix when dealing with nested SPIR-V types, e.g.: !spirv.ptr<!spirv.array<...>> ==> !spirv.ptr<array<...>>.

The abbreviated format should support arbitrary nesting of SPIR-V types.

llvmbot commented 1 year ago

@llvm/issue-subscribers-mlir-spirv

llvmbot commented 7 months ago

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. 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. Detailed instructions can be found in GitHub's documentation.

If you have any further questions about this issue, don't hesitate to ask via a comment in the thread below.

llvmbot commented 7 months ago

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

Author: Jakub Kuderski (kuhar)

We can elide the leading dialect prefix when dealing with nested SPIR-V types, e.g.: `!spirv.ptr<!spirv.array<...>>` ==> `!spirv.ptr<array<...>>`. The abbreviated format should support arbitrary nesting of SPIR-V types.
kuhar commented 7 months ago

Update: I looked at this at some point and this would require writing a bunch of custom parsing/printing code similar to the llvm dialect. There was a proposal presented during the US LLVM Dev / MLIR summit to allow these abbreviated formats in a more general way. I can't find the abstract now, maybe someone like @ftynse or @jpienaar know what I'm talking about.

I think we should put this issue on hold until this generic support in MLIR is in place.