google / google/shaderc

Assembling spvasm doesn't use target env version (API & command line)

Open
#1,045 1 comment 1 reaction 1 assignee Claimed by @dneto0 View on GitHub
bug
Dominant language
C++
Stars
2.2k
Forks
445
Avg merge
11h 6m
Merged PRs (30d)
6

Description

Compiling this shader with `glslc --target-env=vulkan1.2 test.spvasm -o test.spv`:

```
OpCapability Shader
OpMemoryModel Logical GLSL450
OpEntryPoint Fragment %main "main" %gl_FragColor
OpExecutionMode %main OriginUpperLeft
OpSource GLSL 330
OpName %main "main"
OpName %gl_FragColor "gl_FragColor"
OpDecorate %gl_FragColor Location 0
%void = OpTypeVoid
%3 = OpTypeFunction %void
%float = OpTypeFloat 32
%v4float = OpTypeVector %float 4
%_ptr_Output_v4float = OpTypePointer Output %v4float
%gl_FragColor = OpVariable %_ptr_Output_v4float Output
%float_0_400000006 = OpConstant %float 0.400000006
%float_0_800000012 = OpConstant %float 0.800000012
%float_1 = OpConstant %float 1
%13 = OpConstantComposite %v4float %float_0_400000006 %float_0_400000006 %float_0_800000012 %float_1

%14 = OpCopyLogical %v4float %13

%main = OpFunction %void None %3
%5 = OpLabel
OpStore %gl_FragColor %14
OpReturn
OpFunctionEnd
```

Fails because it builds in a vulkan1.0 target environment so the `OpCopyLogical` isn't recognised as an opcode. It also crashes when the assembly fails.

Digging in, the target environment that gets used is the one from `additional_options.target_env_version` in `shaderc_assemble_into_spv`, but I don't see any code that actually ever sets that member. In the API when you call `shaderc_compile_options_set_target_env` it gets set into `options.compiler.target_env_version_` instead. The same thing happens on the command line in glslc with `compiler.options().SetTargetEnvironment(...)`.

In addition when processing assembly files via the command line the options aren't passed into `AssembleToSpv`.

Making the change below seems to fix it below by passing the options in, and also saving target_env_version in `shaderc_compile_options_set_target_env`, but I'm not sure if that's the correct fix or if there shouldn't be this duplication of storing the target env data:

```patch
$ git diff
diff --git a/glslc/src/file_compiler.cc b/glslc/src/file_compiler.cc
index 839cc6e..12a2ff0 100644
--- a/glslc/src/file_compiler.cc
+++ b/glslc/src/file_compiler.cc
@@ -93,8 +93,8 @@ bool FileCompiler::CompileShaderFile(const InputFileSpec& input_file) {
if (input_file.stage == shaderc_spirv_assembly) {
// Only act if the requested target is SPIR-V binary.
if (output_type_ == OutputType::SpirvBinary) {
- const auto result =
- compiler_.AssembleToSpv(source_string.data(), source_string.size());
+ const auto result = compiler_.AssembleToSpv(
+ source_string.data(), source_string.size(), options_);
return EmitCompiledResult(result, input_file.name, output_file_name,
error_file_name, used_source_files);
} else {
diff --git a/libshaderc/src/shaderc.cc b/libshaderc/src/shaderc.cc
index 0421656..c6f1acd 100644
--- a/libshaderc/src/shaderc.cc
+++ b/libshaderc/src/shaderc.cc
@@ -469,6 +469,7 @@ void shaderc_compile_options_set_target_env(shaderc_compile_options_t options,
shaderc_target_env target,
uint32_t version) {
options->target_env = target;
+ options->target_env_version = version;
options->compiler.SetTargetEnv(GetCompilerTargetEnv(target),
GetCompilerTargetEnvVersion(version));
}```

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.