Skip to content

Fix regression in spirv generator #412

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

Closed
wants to merge 3 commits into from
Closed

Conversation

akharche
Copy link
Contributor

Flag for debug was removed in previous prs

@PokhodenkoSA
Copy link
Contributor

PokhodenkoSA commented Jun 1, 2021

Code around is hard to read. Need refactoring. In separate PR?

@PokhodenkoSA
Copy link
Contributor

PokhodenkoSA commented Jun 1, 2021

Is it possible to make test for it?
Manual reproducer?

@PokhodenkoSA PokhodenkoSA mentioned this pull request Jun 1, 2021
1 task
@@ -105,7 +105,7 @@ def generate(self, ipath, opath, llvm_spirv_args):
llvm_spirv_call_args = ["llvm-spirv"]
if llvm_spirv_args is not None:
llvm_spirv_call_args += llvm_spirv_args
llvm_spirv_call_args += ["-o", opath, ipath + ".bc"]
llvm_spirv_call_args += [*llvm_spirv_flags, "-o", opath, ipath + ".bc"]
Copy link
Contributor

@PokhodenkoSA PokhodenkoSA Jun 2, 2021

Choose a reason for hiding this comment

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

I have reverted this change and test_function_with_debuginfo PASSED. Why it could be?
Could you check it too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because (as I said earlier) this flag only fixes error in spirv-val mode.

Copy link
Contributor

Choose a reason for hiding this comment

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

spirv-val mode

Could you enable this mode in test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I can, but checking spirv (spirv-val mode) is blocked by a set of problems. This flag fixes only one problem connected to appropriate Debug name in spirv

Copy link
Contributor

@PokhodenkoSA PokhodenkoSA left a comment

Choose a reason for hiding this comment

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

The test does not test changes.

@akharche
Copy link
Contributor Author

akharche commented Jun 2, 2021

The test does not test changes.

Yes. It tests calling function with debug in general. Currently it is hardly possible to write a test on checking failing in spirv-val mode because there are lots of other problems.

@PokhodenkoSA
Copy link
Contributor

@akharche what is the status now?

@diptorupd diptorupd deleted the akharche/fix_regression branch February 5, 2023 16:40
@diptorupd diptorupd restored the akharche/fix_regression branch February 5, 2023 16:40
@diptorupd diptorupd deleted the akharche/fix_regression branch March 6, 2023 06:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working debug Related to #149 spirv validation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants