Skip to content

Use standard SPIRV-Tools transformation recipes#447

Merged
antiagainst merged 6 commits intogoogle:masterfrom
antiagainst:transformations
Mar 29, 2018
Merged

Use standard SPIRV-Tools transformation recipes#447
antiagainst merged 6 commits intogoogle:masterfrom
antiagainst:transformations

Conversation

@antiagainst
Copy link
Copy Markdown
Contributor

@antiagainst antiagainst commented Mar 22, 2018

Previously we manually manage the list of passes.
This commit converts to use the SPIRV-Tools standard
ones.

@antiagainst antiagainst requested a review from dneto0 March 22, 2018 19:33
@antiagainst
Copy link
Copy Markdown
Contributor Author

antiagainst commented Mar 22, 2018

I haven't added the API parameters for invoking performance passes. I'm not sure what we should call it. shaderc_optimization_level_one, shaderc_optimization_level_three, shaderc_optimization_level_performance?

shaderc/libshaderc/include/shaderc/shaderc.h

Lines 124 to 127 in c993158

typedef enum {
shaderc_optimization_level_zero, // no optimization
shaderc_optimization_level_size, // optimize towards reducing code size
} shaderc_optimization_level;

@antiagainst
Copy link
Copy Markdown
Contributor Author

antiagainst commented Mar 22, 2018

Anyway, that can happen in a following up PR. This PR is basically internal clean up.

@ehsannas
Copy link
Copy Markdown
Contributor

ehsannas commented Mar 22, 2018

Looks like the bots ran before my fixes went in.

@antiagainst antiagainst mentioned this pull request Mar 22, 2018
Closed
@antiagainst antiagainst mentioned this pull request Mar 23, 2018
Closed
@antiagainst
Copy link
Copy Markdown
Contributor Author

antiagainst commented Mar 23, 2018

Okay, added shader_optimization_level_performance into the API and -O for glslc.

@chrisvarns
Copy link
Copy Markdown

chrisvarns commented Mar 23, 2018 •
edited
Loading

Just tried this out, FYI shaderc_compile_options_set_optimization_level in libshaderc/src/shaderc.cc is missing the switch for OptimizationLevel::Performance atm.

EDIT: ditto the called method in libshaderc_util/src/compiler.cc.

@antiagainst
Copy link
Copy Markdown
Contributor Author

antiagainst commented Mar 23, 2018

@chrisvarns: Good catch! Thanks for pointing out! It turned out to be that we always run legalization passes for HLSL source code so my tests, using HLSL, does not reveal the problem previously. Changed to GLSL to be clear.

dneto0
dneto0 requested changes Mar 23, 2018
View reviewed changes
libshaderc/include/shaderc/shaderc.h Outdated
typedef enum {
shaderc_optimization_level_zero, // no optimization
shaderc_optimization_level_zero, // no optimization
shaderc_optimization_level_performance, // optimize towards performance
Copy link
Copy Markdown
Collaborator

@dneto0 dneto0 Mar 23, 2018

Choose a reason for hiding this comment

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

Don't insert a value in between two existing values. That breaks compatibility with anybody who has code that is compiled against the old header. Their code might ask for size optimization and now they would end up getting performance.

Copy link
Copy Markdown
Contributor Author

@antiagainst antiagainst Mar 23, 2018

Choose a reason for hiding this comment

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

Urgh. Yes. I don't know what I was doing...

libshaderc_util/include/libshaderc_util/compiler.h Outdated
Zero, // No optimization.
Size, // Optimization towards reducing code size.
Zero, // No optimization.
Performance, // Optimization towards better performance.
Copy link
Copy Markdown
Collaborator

@dneto0 dneto0 Mar 23, 2018

Choose a reason for hiding this comment

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

Keep these in same order as the C header. So reorder to match requested reorder there.

Copy link
Copy Markdown
Contributor Author

@antiagainst antiagainst Mar 23, 2018

Choose a reason for hiding this comment

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

Fixed.

@antiagainst antiagainst merged commit 2a0f3a3 into google:master Mar 29, 2018
@antiagainst antiagainst deleted the transformations branch March 29, 2018 15:01
stenzek added a commit to stenzek/shaderc that referenced this pull request Feb 2, 2025
@stenzek
Squashed 'third_party/spirv-headers/' changes from 2a9b6f951c..e7294a…
3e02cc6 
…8ebe

e7294a8ebe Add headers for SPV_NV_linear_swept_spheres. (google#483)
003bcf4e0d Add headers for SPV_NV_cluster_acceleration_structure. (google#484)
43764cc756 updates IntegerFunctions2INTEL to remove Shader capability dependency (google#481)
767e901c98 Add SPV_NV_cooperative_vector (google#482)
2b2e05e088 grammar and header changes for SPV_INTEL_subgroup_matrix_multiply_accumulate (google#471)
0659679d96 Add a source language for Rust (google#472)
9ca0e67b5e grammar and header changes for SPV_INTEL_2d_block_io (google#470)
a380cd2543 Fix OpAsmTargetINTEL operand (google#468)
3f17b2af67 [SPIRV] Add generator magic number (google#467)
36d5e2ddaa Add provisional key to grammar (google#464)
45b314049d Add NonSemanticShaderDebugInfo100.h to bazel build. (google#466)
2ce05a6f79 Remove trailing whitespace (google#465)
996c728cf7 add basic utility code testing for cpp, cpp11, and c (google#461)
cb6b2c32db Fix on header generator for c++11, regenerated products (google#463)
22c4d1b1e9 Add SPV_NV_cooperative_matrix2 and SPV_NV_tensor_addressing (google#458)
252dc2df08 Add nuvk's spirv emitter. (google#454)
50bc4debdc VkspReflection non-sematic: remove literals for Ids (google#453)
07ddb1c0f1 Update SPV_AMDX_shader_enqueue (google#452)
d92cf88c37 Add "aliases" fields to the grammar and remove duplicated (google#447)
a62b032007 Add SPV_EXT_arithmetic_fence (google#450)
ec59c77a3b Reserve SPIR-V enums for MediaTek (google#451)
0413bc33fa Add SPV_EXT_optnone (google#449)

git-subtree-dir: third_party/spirv-headers
git-subtree-split: e7294a8ebed84f8c5bd3686c68dbe12a4e65b644
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dneto0 dneto0 dneto0 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@antiagainst @ehsannas @chrisvarns @dneto0