Skip to content

Move helper functions out of SemaHLSL into a common location for SemaSPIRV to use #123831

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

Open
Icohedron opened this issue Jan 21, 2025 · 7 comments
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema"

Comments

@Icohedron
Copy link
Contributor

Icohedron commented Jan 21, 2025

While working on #122992, @inbelic mentioned that the semantics checks in clang/lib/Sema/SemaSPIRV.cpp could be simplified by using helper functions that are currently only present in clang/lib/Sema/SemaHLSL.cpp, such as CheckArgTypeIsCorrect and CheckAllArgTypesAreCorrect.

It would be useful to migrate the helper functions out of SemaHLSL into a common location accessible to both SemaHLSL and SemaSPIRV to make the code in SemaSPIRV less verbose and more readable.

@farzonl
Copy link
Member

farzonl commented Jan 21, 2025

Keep in mind not all hlsl vector rules apply to spirv. For example half and double data type rules in hlsl will be different from spirv.

@damyanp damyanp added the clang:frontend Language frontend issues, e.g. anything involving "Sema" label Jan 28, 2025
@llvmbot
Copy link
Member

llvmbot commented Jan 28, 2025

@llvm/issue-subscribers-clang-frontend

Author: Deric Cheung (Icohedron)

While working on #122992, @inbelic [mentioned](https://github.com//pull/122992#pullrequestreview-2559625948) that the semantics checks in `clang/lib/Sema/SemaSPIRV.cpp` could be simplified by using helper functions that are currently only present in `clang/lib/Sema/SemaHLSL.cpp`, such as `CheckArgTypeIsCorrect` and `CheckAllArgTypesAreCorrect`.

It would be useful to migrate the helper functions out of SemaHLSL into a common location accessible to both SemaHLSL and SemaSPIRV to make the code in SemaSPIRV less verbose and more readable.

@damyanp damyanp added the good first issue https://github.com/llvm/llvm-project/contribute label Jan 28, 2025
@damyanp damyanp changed the title [HLSL] Move helper functions out of SemaHLSL into a common location for SemaSPIRV to use Move helper functions out of SemaHLSL into a common location for SemaSPIRV to use Jan 28, 2025
@llvmbot
Copy link
Member

llvmbot commented Jan 28, 2025

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. Check that no other contributor has already been assigned to this issue. If you believe that no one is actually working on it despite an assignment, ping the person. After one week without a response, the assignee may be changed.
  2. In the comments of this issue, request for it to be assigned to you, or just create a pull request after following the steps below. Mention this issue in the description of the pull request.
  3. Fix the issue locally.
  4. 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.
  5. Create a Git commit.
  6. Run git clang-format HEAD~1 to format your changes.
  7. Open a pull request to the upstream repository on GitHub. Detailed instructions can be found in GitHub's documentation. Mention this issue in the description of the pull request.

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

@llvmbot
Copy link
Member

llvmbot commented Jan 28, 2025

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

Author: Deric Cheung (Icohedron)

While working on #122992, @inbelic [mentioned](https://github.com//pull/122992#pullrequestreview-2559625948) that the semantics checks in `clang/lib/Sema/SemaSPIRV.cpp` could be simplified by using helper functions that are currently only present in `clang/lib/Sema/SemaHLSL.cpp`, such as `CheckArgTypeIsCorrect` and `CheckAllArgTypesAreCorrect`.

It would be useful to migrate the helper functions out of SemaHLSL into a common location accessible to both SemaHLSL and SemaSPIRV to make the code in SemaSPIRV less verbose and more readable.

@damyanp damyanp removed the HLSL HLSL Language Support label Jan 28, 2025
@damyanp damyanp moved this from Planning to Ready in HLSL Support Jan 28, 2025
@bassiounix
Copy link
Contributor

I'd like to be assigned to this. I'll try to extract the common part with the 2 functions mentioned in the issue and the SemaSPIRV.cpp.

@kunxl-gg
Copy link

Hey, is this issue being worked upon by someone? Would like to work on this.

@farzonl
Copy link
Member

farzonl commented Mar 13, 2025

@kunxl-gg this issue is blocked. We should remove the good first issue from this ticket. The last pr revealed this was a more complicated ask than we had considered.

@farzonl farzonl removed the good first issue https://github.com/llvm/llvm-project/contribute label Mar 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema"
Projects
Status: Ready
Development

No branches or pull requests

7 participants