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

spvTextToBinary to return a mapping of textual ID names to numeric IDs #41

Closed
umar456 opened this issue Dec 3, 2015 · 8 comments
Closed

Comments

@umar456
Copy link
Contributor

umar456 commented Dec 3, 2015

Use Case:
Unit test in the validation rely on the error codes validate the result of the spvValidate function. Ideally the tests should check both the error code and the diagnostic message to verify that the test case has accurately passed. This is currently not possible because the named ID representation is lost during the Text to Binary conversion.

The spvTextToBinary function should return a map which contains a mapping from ID to ID names used in the textual representation of the SPIR-V.

@umar456 umar456 changed the title spvTextToBinary to return a mapping of variables to IDs spvTextToBinary to return a mapping of textual ID names to numeric IDs Dec 3, 2015
@dekimir
Copy link

dekimir commented Dec 3, 2015

This would be nice to have, though it's not essential for checking the diagnostic messages even now. The message text around %IDs can be checked, and OpName instructions (where present) could be used to map %IDs to source names.

@umar456
Copy link
Contributor Author

umar456 commented Dec 3, 2015

Good point. I should be able to create diagnostic messages based on the (OpName) commands and verify the output using this approach.

@dneto0
Copy link
Collaborator

dneto0 commented Dec 3, 2015

Keep in mind that OpName instructions are optional.
Also, the validator is checking binaries coming from any front end, which could be much more than the assembler in SPIR-V Tools.
So I see using the OpName values in validator diagnostics as nice-to-have, but not essential.

@dekimir
Copy link

dekimir commented Dec 3, 2015

Similarly, not every front end can furnish spvValidate() with a %ID mapping
proposed in OP.

On Thu, 3 Dec 2015 at 14:43 David Neto notifications@github.com wrote:

Keep in mind that OpName instructions are optional.
Also, the validator is checking binaries coming from any front end, which
could be much more than the assembler in SPIR-V Tools.
So I see using the OpName values in validator diagnostics as nice-to-have,
but not essential.


Reply to this email directly or view it on GitHub
#41 (comment)
.

Sent from a mobile device.

@umar456
Copy link
Contributor Author

umar456 commented Dec 3, 2015

I understand. I was plan on formatting the diagnostic message to display the id and only display the name if an OpName command references the ID. For example

No OpName

ID 32 has not been defined

With OpName

ID 32[foo] has not been defined

This should be pretty easy to implement. I can then use regex to parse the message to verify the unit tests.

@dneto0
Copy link
Collaborator

dneto0 commented Dec 3, 2015

Yes, that would work.

Alternately, you could use the same strategy the assembler tests use: rely on the fact that the assembler assigns Id indices via a counter, starting at 1.

See, for example, some disassembler diagnostics tests at https://github.com/KhronosGroup/SPIRV-Tools/blob/master/test/BinaryToText.cpp#L181
Those tests first assemble some text, then disassemble text but fail with a message. (They use TextToBinaryTestBase::EncodeSuccessfullyDecodeFailed)

@umar456
Copy link
Contributor Author

umar456 commented Dec 5, 2015

That sounds like it would be dependent on the implementation. I wouldn't feel comfortable writing tests like that. It could also become complicated once we test more complicated cases.

dgkoch pushed a commit to dgkoch/SPIRV-Tools that referenced this issue Nov 21, 2018
Reserve number 16 to Mesa-IR/SPIR-V Translator
@dj2
Copy link
Contributor

dj2 commented Sep 26, 2020

I'm not sure if there is anything to do here,spirv-dis does the name mapping when possible already. Can this be closed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants