Skip to content

Add support for SPV_KHR_non_semantic_info#3109

Closed
baldurk wants to merge 6 commits intoKhronosGroup:masterfrom
baldurk:spv-khr-non-semantic-info
Closed

Add support for SPV_KHR_non_semantic_info#3109
baldurk wants to merge 6 commits intoKhronosGroup:masterfrom
baldurk:spv-khr-non-semantic-info

Conversation

@baldurk
Copy link
Copy Markdown
Contributor

@baldurk baldurk commented Dec 18, 2019

No description provided.

@baldurk
Add support for SPV_KHR_non_semantic_info
a5e3a93 
This entails a couple of changes:

- Allowing unknown OpExtInstImport that begin with the prefix `NonSemantic.`
- Allowing OpExtInst that reference any of those sets to contain unknown
  ext inst instruction numbers, and assume the format is always a series of IDs
  as guaranteed by the extension.
- Allowing those OpExtInst to appear in the types/variables/constants section.
- Not stripping OpString in the --strip-debug pass, since it may be referenced
  by these non-semantic OpExtInsts.
- Stripping them instead in the --strip-reflect pass.
@baldurk
Update wording of validation message
968ec21
@baldurk
Add adjacency validation of non-semantic OpExtInst
6bd4cd9 
- We validate and test that OpExtInst cannot appear before or between
  OpPhi instructions, or before/between OpFunctionParameter
  instructions.
@baldurk
Change non-semantic extinst type to single value
e704eaf 
* Add helper function spvExtInstIsNonSemantic() which will check if the extinst
  set is non-semantic or not, either the unknown generic value or any future
  recognised non-semantic set.
@baldurk
Add test of a complex non-semantic extinst
c6aa65d
@baldurk
Use DefUseManager in StripDebugInfoPass to strip some OpStrings
f344ceb 
* Any OpString used by a non-semantic instruction cannot be stripped, all others
  can so we search for uses to see if each string can be removed.
* We only do this if the non-semantic debug info extension is enabled, otherwise
  all strings can be trivially removed.
@dneto0
Copy link
Copy Markdown
Collaborator

dneto0 commented Dec 18, 2019

Hey @afd the build failures here seem to be an overrides warning for protobufs code (see below). That's unrelated to this PR. Please take a look.

[190/885] Building CXX object external/protobuf/cmake/CMakeFiles/libprotobuf.dir/__/src/google/protobuf/util/internal/default_value_objectwriter.cc.o
In file included from ../external/protobuf/src/google/protobuf/util/internal/default_value_objectwriter.cc:31:
../external/protobuf/src/google/protobuf/util/internal/default_value_objectwriter.h:122:37: warning: 'RenderNull' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
virtual DefaultValueObjectWriter* RenderNull(StringPiece name);
^
../external/protobuf/src/google/protobuf/util/internal/object_writer.h:105:25: note: overridden virtual function is here
virtual ObjectWriter* RenderNull(StringPiece name) = 0;

@dneto0
Copy link
Copy Markdown
Collaborator

dneto0 commented Dec 18, 2019

Looks like there are some legit endogenous issues.
E.g. GCC release build has these errors:

867/889] Building CXX object test/val/CMakeFiles/test_val_fghijklmnop.dir/val_non_semantic_test.cpp.o
FAILED: test/val/CMakeFiles/test_val_fghijklmnop.dir/val_non_semantic_test.cpp.o
/usr/bin/c++ -DSPIRV_CHECK_CONTEXT -DSPIRV_COLOR_TERMINAL -DSPIRV_LINUX -DSPIRV_TIMER_ENABLED -I../ -I../external/spirv-headers/include -I../include -I../test -I. -isystem ../external/googletest/googletest/include -isystem ../external/googletest/googlemock/include -I../external/effcee -I../external/effcee/effcee/.. -I../external/re2 -isystem ../external/googletest/googlemock -isystem ../external/googletest/googletest -O2 -g -DNDEBUG -fPIE -Wall -Wextra -Wnon-virtual-dtor -Wno-missing-field-initializers -Werror -std=c++11 -fno-exceptions -fno-rtti -Wno-long-long -Wshadow -Wundef -Wconversion -Wno-sign-conversion -Wno-undef -Wno-shadow -std=gnu++11 -MD -MT test/val/CMakeFiles/test_val_fghijklmnop.dir/val_non_semantic_test.cpp.o -MF test/val/CMakeFiles/test_val_fghijklmnop.dir/val_non_semantic_test.cpp.o.d -o test/val/CMakeFiles/test_val_fghijklmnop.dir/val_non_semantic_test.cpp.o -c /tmpfs/src/github/SPIRV-Tools/test/val/val_non_semantic_test.cpp
/tmpfs/src/github/SPIRV-Tools/test/val/val_non_semantic_test.cpp:128:69: error: unterminated raw string
Combine(Values(true), Values(true), Values(R"(
^
/tmpfs/src/github/SPIRV-Tools/test/val/val_non_semantic_test.cpp:129:74: error: missing terminating " character [-Werror]
%result = OpExtInst %void %extinst 123 %i32 %u32_2 %f32vec4_1234 %u32_0)"),
^
/tmpfs/src/github/SPIRV-Tools/test/val/val_non_semantic_test.cpp:127:1: error: stray ‘R’ in program
INSTANTIATE_TEST_SUITE_P(ComplexGlobalExtInst, ValidateNonSemanticGenerated,
^
/tmpfs/src/github/SPIRV-Tools/test/val/val_non_semantic_test.cpp:127:1: error: missing terminating " character
/tmpfs/src/github/SPIRV-Tools/test/val/val_non_semantic_test.cpp:127:1: error: stray ‘R’ in program
/tmpfs/src/github/SPIRV-Tools/test/val/val_non_semantic_test.cpp:127:1: error: missing terminating " character
/tmpfs/src/github/SPIRV-Tools/test/val/val_non_semantic_test.cpp:50:15: error: unused parameter ‘env’ [-Werror=unused-parameter]
CodeGenerator GetNonSemanticCodeGenerator(spv_target_env env,

@dneto0
Copy link
Copy Markdown
Collaborator

dneto0 commented Dec 18, 2019

Hm. That raw string issue might be an bad interaction with googletest. I've seen that before. Should be easy for me to fix.

@dneto0
Copy link
Copy Markdown
Collaborator

dneto0 commented Dec 18, 2019

It's also a pretty old GCC. GNU 4.8.5

@dneto0 dneto0 self-requested a review December 18, 2019 19:09
@baldurk
Copy link
Copy Markdown
Contributor Author

baldurk commented Dec 18, 2019

Is there anything you want me to fixup on the branch? That raw string could probably be regular literal, I think I deleted the second line I had that prompted it to be a raw literal.

@dneto0 dneto0 mentioned this pull request Dec 18, 2019
Merged
@dneto0
Copy link
Copy Markdown
Collaborator

dneto0 commented Dec 18, 2019

Is there anything you want me to fixup on the branch? That raw string could probably be regular literal, I think I deleted the second line I had that prompted it to be a raw literal.

Not just yet.
I'm experimenting with some patches on another PR #3110
I have a few more hours to try.

@dneto0
Copy link
Copy Markdown
Collaborator

dneto0 commented Dec 18, 2019

This content was merged via #3110

@dneto0 dneto0 closed this Dec 18, 2019
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.

2 participants

@baldurk @dneto0