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

Use SPIR-V headers from the KhronosGroup/SPIRV-Headers repo. #186

Closed
wants to merge 1 commit into from

Conversation

antiagainst
Copy link
Contributor

No description provided.

@@ -54,9 +54,9 @@ We intend to maintain a linear history on the GitHub `master` branch.
* `external/googletest`: Intended location for the
[googletest][googletest] sources, not provided
* `include/`: API clients should add this directory to the include search path
* `external/SPIRV-Headers`: Intended location for
[SPIR-V headers][spirv-headers], not provided
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is the second one lower case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's just ref'ing the footnote. :)

@dneto0
Copy link
Collaborator

dneto0 commented Apr 18, 2016

In principle, this is good.

Don't we need a corresponding change in google/shaderc to react to the rename of the .inc file?

Also, we need to make an AOSP repository for SPIRV-Headers, and update the manifest for the NDK to include it.

-1 Can't include this change until the above are staged as well.

@antiagainst
Copy link
Contributor Author

@dneto0: we don't need to do anything in google/shaderc for .inc files, but we do need to add build steps to pull SPIRV-Headers there.

See google/shaderc#203.

@@ -161,23 +168,16 @@ function(spvtools_opencl_tables VERSION)
add_custom_target(spirv-tools-build-opencl-tables-${VERSION}
${PYTHON_EXECUTABLE}
${CMAKE_CURRENT_SOURCE_DIR}/utils/generate_grammar_tables.py
--spirv-core-grammar=${spirv-tools_SOURCE_DIR}/source/spirv-${VERSION}.core.grammar.json
--spirv-core-grammar=${SPIRV_HEADER_INCLUDE_DIR}/spirv/${VERSION}/spirv.core.grammar.json
Copy link

Choose a reason for hiding this comment

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

Don't forget to rebase this against #183 (or the other way around).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rebased.

@dekimir
Copy link

dekimir commented Apr 18, 2016

LGTM, though still gated on AOSP work...

@antiagainst antiagainst force-pushed the header-repo branch 2 times, most recently from 78141e7 to 0f92d7a Compare April 23, 2016 00:04
@dneto0
Copy link
Collaborator

dneto0 commented Jun 1, 2016

LGTM.
Question for @dekimir : Why is this gated on AOSP work? The AOSP build only takes in new sources when we explicitly merge into its master.

@dekimir
Copy link

dekimir commented Jun 1, 2016

@dneto0: at the time of my comment, the manifest branch still hadn't included SPIRV-Headers, so #includes would have failed.

@dneto0
Copy link
Collaborator

dneto0 commented Jun 8, 2016

@antiagainst : please rebase

@antiagainst
Copy link
Contributor Author

@dneto0: done

@dneto0
Copy link
Collaborator

dneto0 commented Jun 9, 2016

Sorry, needs another rebase.

@antiagainst
Copy link
Contributor Author

@dneto0: done

@dneto0
Copy link
Collaborator

dneto0 commented Jun 10, 2016

+2

@dneto0
Copy link
Collaborator

dneto0 commented Jun 10, 2016

Rebased and pushed into master as 10dba91

@dneto0 dneto0 closed this Jun 10, 2016
@antiagainst antiagainst deleted the header-repo branch June 10, 2016 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants