Support setting target environment version, e.g. Vulkan 1.1 by dneto0 · Pull Request #445 · google/shaderc

dneto0 · 2018-03-18T06:34:32Z

NOTE: We no longer can compile OpenGL compatibility profile shaders to SPIR-V.
These will error out.

C and C++ API: Set nontrivial target environment version

Glslc --target-env accepts optional version suffix, e.g. vulkan1.1

NOTE: We no longer can compile OpenGL compatibility profile shaders to SPIR-V. These will error out. C and C++ API: Set nontrivial target environment version Glslc --target-env accepts optional version suffix, e.g. vulkan1.1

dneto0 · 2018-03-21T04:01:32Z

FYI: CI-ubuntu-clang-debug failure is a config failure: " clang is not a full path and was not found in the PATH."

dneto0 · 2018-03-21T04:02:32Z

PTAL @antiagainst .

@dgkoch has asked for this to land sooner rather than later.

antiagainst

Sorry for the late review! Overall LGTM! Just a few nits.

antiagainst · 2018-03-21T12:48:44Z

glslc/test/option_target_env.py

+
+
+@inside_glslc_testsuite('OptionTargetEnv')
+class TestTargetEnvEqVulkan1_0WithVulkan1_1ShaderFails(expect.ErrorMessageSubstr):


This is a duplicate test with the one at Line 98?

Yes! Deleting the one at line 98.

antiagainst · 2018-03-21T12:49:45Z

glslc/test/option_target_env.py

+@inside_glslc_testsuite('OptionTargetEnv')
+class TestTargetEnvEqVulkan1_1WithVulkan1_0ShaderSucceeds(expect.ValidObjectFile):
+    shader = FileShader(vulkan_vertex_shader(), '.vert')
+    glslc_args = ['--target-env=vulkan1.0', '-c', shader]


Should be --target-env=vulkan1.1?

antiagainst · 2018-03-21T12:50:48Z

libshaderc/include/shaderc/shaderc.h

-  shaderc_target_env_opengl,         // create SPIR-V under OpenGL semantics
+  shaderc_target_env_vulkan,  // create SPIR-V under Vulkan semantics
+  shaderc_target_env_opengl,  // create SPIR-V under OpenGL semantics
+  // NOTE: SPIR-V coide generation is not supported for shaders under OpenGL


s/coide/code/

antiagainst · 2018-03-21T12:51:40Z

libshaderc/include/shaderc/shaderc.h

+  shaderc_env_version_vulkan_1_0 = (((uint32_t)1 << 22)),
+  shaderc_env_version_vulkan_1_1 = (((uint32_t)1 << 22) | (1 << 12)),
+  // For OpenGL, use the number from #version in shaders.
+  // TODO(dneto): Currently no difference between OpenGL 4.5 andf 4.6.


s/andf/and/

antiagainst · 2018-03-21T12:56:14Z

libshaderc_util/include/libshaderc_util/compiler.h

    OpenGLCompat,
  };

+  // Target environment versions.  These bnumbgers match those used by Glslang.


s/bnumbgers/numbers/

antiagainst · 2018-03-21T12:56:49Z

libshaderc_util/include/libshaderc_util/compiler.h

-  // Sets the target environment.
-  void SetTargetEnv(TargetEnv env);
+  // Sets the target environment, including version.
+  // 0 means the default version for the


for the target environment?

yes. whoops.

antiagainst · 2018-03-21T13:03:19Z

libshaderc_util/src/compiler.cc

  std::vector<uint32_t>& compilation_output_data = std::get<1>(result_tuple);
  size_t& compilation_output_data_size_in_bytes = std::get<2>(result_tuple);

+  // Check target envirohnment.


s/envirohnment/environment/

dgkoch · 2018-03-21T13:06:33Z

libshaderc_util/include/libshaderc_util/compiler.h

    OpenGLCompat,
  };

+  // Target environment versions.  These bnumbgers match those used by Glslang.


"bnumbgers" typo?

dgkoch · 2018-03-21T13:06:55Z

libshaderc_util/include/libshaderc_util/compiler.h


+  // Target environment versions.  These bnumbgers match those used by Glslang.
+  enum class TargetEnvVersion : uint32_t {
+    // For Vulkan, use numbering schmee from vulkan.h


dgkoch · 2018-03-21T13:07:13Z

libshaderc_util/include/libshaderc_util/compiler.h

+  enum class TargetEnvVersion : uint32_t {
+    // For Vulkan, use numbering schmee from vulkan.h
+    Vulkan_1_0 = ((1 << 22)),              // Default to Vuilkan 1.0
+    Vulkan_1_1 = ((1 << 22) | (1 << 12)),  // Default to Vuilkan 1.0


dgkoch · 2018-03-21T13:07:32Z

libshaderc_util/include/libshaderc_util/compiler.h

+    // For Vulkan, use numbering schmee from vulkan.h
+    Vulkan_1_0 = ((1 << 22)),              // Default to Vuilkan 1.0
+    Vulkan_1_1 = ((1 << 22) | (1 << 12)),  // Default to Vuilkan 1.0
+    // For Op[enGL, use the numbering from #version in shaders.


dgkoch · 2018-03-21T13:09:48Z

libshaderc_util/include/libshaderc_util/compiler.h

  enum class TargetEnv {
-    Vulkan,
-    OpenGL,
+    Vulkan,  // Default to Vuilkan 1.0


oh my. I must have been tired. Actually I was on a plane and comments were dark blue on black... :-(

antiagainst · 2018-03-21T20:02:48Z

Also, we should probably update the glslc manual about the changes.

antiagainst · 2018-03-21T20:20:35Z

glslc/src/main.cc

+                    the client version.  Values are:
+                        vulkan1.0       # The default
+                        vulkan1.1
+                        vulkan          # Same as vulkan1.0


A question about the vulkan parameter suddenly comes to me.

What's our guarantee of this parameter? Are we gonna change what it aliases to when, say, Vulkan 1.2 or 1.N releases?

If not, that effectively makes it just a way of saying vulkan1.0, which would be handy for now but in the future developers will mostly want vulkan 1.N. So it will likely be less useful.

If we would like to change it when newer version of Vulkan come out, then how we decide which version to alias to since we are not alias to the newest version here?

Also note that when we have a python or other binary or library without version number in the system, they alias to the newest ones. If you want to the older ones, you need to specify python2.6 or something. I'm not sure whether that's particularly related here, but seems it's a convention?

Okay, I think maybe make vulkan alias to the previous major release is a good balance between new functionality and driver stability?

JohnK and I independently came to the same conclusion that at least for now defaulting to Vulkan 1.0 is a sane default. It also happens to retain backward compatibility.
It is a judgement call though.

That's fair.

- Update glslc manual - Update comments at the shaderc_compile_options_set_target_env entry point and its moral equivalents - Fix typos in comments - Remove duplicate test - Fix 'vulkan1.1' option in test

dneto0 · 2018-03-22T06:59:08Z

Thanks for the reviews. I've addressed all feedback, and also updated the comments at shaderc_compile_set_target_env and its friends.

@antiagainst PTAL

antiagainst · 2018-03-22T14:15:33Z

Appveyor & Travis is fine. I'll ignore Google bots for now.

ehsannas · 2018-03-22T14:18:40Z

The Google bot failures are new and unrelated to this CL. I will work on fixing them.

…951c 2a9b6f951c Add Capability and Execution mode SPV_KHR_compute_shader_derivatives (google#446) efb6b4099d add support for SPV_INTEL_subgroup_buffer_prefetch (google#442) 744753a21d Update spirv generator info (google#445) 69ab0f32dc Add UnormInt2_101010EXT ImageChannelDataType (google#444) 1b75a4ae0b Request a vendor id for Kongruent (google#443) git-subtree-dir: third_party/spirv-headers git-subtree-split: 2a9b6f951c7d6b04b6c21fe1bf3f475b68b84801

dneto0 requested a review from antiagainst March 18, 2018 06:34

Support setting target environment version, e.g. Vulkan 1.1

bed809f

NOTE: We no longer can compile OpenGL compatibility profile shaders to SPIR-V. These will error out. C and C++ API: Set nontrivial target environment version Glslc --target-env accepts optional version suffix, e.g. vulkan1.1

dneto0 force-pushed the vulkan-1.1 branch from ed97001 to bed809f Compare March 21, 2018 03:55

antiagainst suggested changes Mar 21, 2018

View reviewed changes

dgkoch reviewed Mar 21, 2018

View reviewed changes

antiagainst reviewed Mar 21, 2018

View reviewed changes

Fixes due to code review

c5e24d1

- Update glslc manual - Update comments at the shaderc_compile_options_set_target_env entry point and its moral equivalents - Fix typos in comments - Remove duplicate test - Fix 'vulkan1.1' option in test

antiagainst approved these changes Mar 22, 2018

View reviewed changes

antiagainst merged commit c993158 into google:master Mar 22, 2018

dgkoch mentioned this pull request Mar 22, 2018

API for setting target env should allow switching between Vulkan 1.0 an Vulkan 1.1 #442

Closed

dneto0 mentioned this pull request Apr 19, 2018

WIP: Force SPIR-V 1.0 binaries #416

Closed



		@inside_glslc_testsuite('OptionTargetEnv')
		class TestTargetEnvEqVulkan1_0WithVulkan1_1ShaderFails(expect.ErrorMessageSubstr):

Conversation

dneto0 commented Mar 18, 2018

Uh oh!

dneto0 commented Mar 21, 2018

Uh oh!

dneto0 commented Mar 21, 2018

Uh oh!

antiagainst left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antiagainst commented Mar 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dneto0 commented Mar 22, 2018

Uh oh!

antiagainst commented Mar 22, 2018

Uh oh!

ehsannas commented Mar 22, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants