Basic SSA Validation #27

umar456 · 2015-11-23T23:51:17Z

Updated validation tests to assignment form
Fixed function tests to handle multiple parameters
Basic SSA Validation

AWoloszyn · 2015-11-24T15:00:05Z

source/validate.cpp

      if (SPV_OPERAND_TYPE_LITERAL_STRING == type) {
        spvCheckReturn(spvValidateOperandsString(
            words + index, wordCount - index, position, pDiagnostic));
        // NOTE: String literals are always at the end of Opcodes
        break;
      } else if (SPV_OPERAND_TYPE_LITERAL_INTEGER == type) {
-        spvCheckReturn(spvValidateOperandsLiteral(
-            words + index, wordCount - index, 2, position, pDiagnostic));
+        // spvCheckReturn(spvValidateOperandsNumber(


If you plan on re-enabling this code, then use:
#if 0
#endif
Around disabled code, and add a TODO(): to re-enable at some point

Otherwise just delete the code.

Fixed da35d28

dneto0 · 2015-12-03T21:31:34Z

test/Validate.SSA.cpp

+
+INSTANTIATE_TEST_CASE_P(KernelArgs, Validate, ::testing::ValuesIn(cases));
+
+string SetOps(string &str, pair<string, bool> param) {


const string &str

dneto0 · 2015-12-03T21:44:46Z

Ok, finished review of latest commit.

Must fix the index == 8 bug in checking for allowable forward use of Id with OpFunction
Should fix tests so they only fail for the reason being tested. A lots of the test cases are invalid for multiple reasons, even though the validator will only check on of those reasons right now. For example a lot of the tests are missing OpMemoryModel, or have OpConstant or OpTypeInt that occur after the first OpFunction instruction.
Nice to fix: Simplify tests to remove extraneous stuff. E.g. a lot of Id checks can be done without any function bodies at all. I've suggested one in detail.
Quibble: Some naming things. Like instead of "...MissingLabel..." say "...MissingTarget..." since "Label" is already a first class concept in SPIR-V.

(and maybe other stuff I've forgotten by now.)

dneto0 · 2015-12-03T21:52:20Z

Oh, and I forgot to thank you for making thorough checks for invalidity of each ID operand that can't be forward declared!

dneto0 · 2015-12-04T16:34:28Z

test/Validate.SSA.cpp

+
+  if (!param.second) {
+    regex ndrange_regex("[^\n]%ndval");
+    out2 = regex_replace(out, ndrange_regex, "");


This doesn't compile with GCC g++ 4.8.4

Validate.SSA.cpp:592:48: error: no matching function for call to ‘regex_replace(std::string&, std::regex&, const char [1])’
and later details say template argument deduction/substitution failed.

What compilers are we targeting?

VisualStudio 2013. (e.g. no constexpr)
GCC 4.8.2 ; that's what comes with Ubunto 14.04 Trusty LTS.
A recent Clang, but I expect that will be at least as feature rich as the GCC.

I should document this in the README. :-(

FYI. The Chromium project notes gotchas with various compiler's C++11 support:
https://chromium-cpp.appspot.com/
E.g. you can see there that constexpr isn't supported in VisualStudio 2013
But we use more features of C++11 than Chromium allows, I think.

Can I add a dependency to boost.regex or boost.xpressive?

* Update fixture to only run certian validation passes * Fixtures now return error codes * Fix fwd index for SpvOpEnqueueKernel * Add a SPV_VALIDATE_SSA_BIT for testing purposes * Enable several tests failing in ID validation * Fix OpEnqueueKernel usage (missing flag arg) * Formatting SPIR-V * Simplify test cases

umar456 · 2015-12-06T04:38:45Z

Addressed the feedback thus far.

umar456 · 2015-12-08T15:50:37Z

Added additional unit tests (OpPhi, OpLoopMerge)
Added support for OpName based diagnostics
Unit tests now check for id names in diagnostic message

dneto0 · 2015-12-08T20:28:27Z

Whoops, I was reviewing an older commit. Still looking...

dneto0 · 2015-12-08T20:57:56Z

Done this round.
Issues:

Fix the block termination in the OpPhi tests
Any ID operand to OpLoopMerge can be forward declared. Either add a TODO for tests for OpLoopMerge or add tests themselves.
Missing blurbs in a couple of places. (May as well do it in the PR.)
Also add #include to fix compilation on GCC.

Recommend not adding new tests to this PR because they can potentially require further rounds of review.

umar456 · 2015-12-09T17:10:59Z

Addressed feedback so far.

dneto0 · 2015-12-09T19:48:03Z

test/Validate.SSA.cpp

+    )";
+  CompileSuccessfully(str);
+  ASSERT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions());
+  ASSERT_TRUE(ContainsString(getDiagnosticString(), "missing"));


OPTIONAL: Sorry I didn't think of this before. But we tend to use gmock matchers, so this would be:

ASSERT_THAT(getDiagnosticString(), HasSubstr("missing"));

(assuming early in the file you have:

#include "gmock/gmock.h" using ::testing::HasSubstr

)

See https://github.com/google/googletest/blob/master/googlemock/docs/CheatSheet.md

It can be very powerful since there are so many predicates to apply. In particular, it's great at matching containers, with very good messages when things fail.

Good for future tests you write.

dneto0 · 2015-12-09T20:00:41Z

I'm satisfied with this for now. I plan to rebase and squash before pushing onto master.

Also, I may have a few more OpPhi cases in mind. But they'll be fore later CLs.

dneto0 · 2015-12-09T20:00:45Z

+2

dneto0 · 2015-12-09T21:26:48Z

Rebased, and made the following changes:

Avoid std::to_string because that's not available on Android. Reworked some local code to use std::strinstream instead.
Did "using std::placeholders::_1" instead of blanket "using namespace std::placeholders" as per Google C++ style guide
Wrapped the local functions in anonymous namespace. Avoids polluting the global namespace.
For diagnostics generation, changed a C-style cast to uint64_t to static cast to size_t, to fix compilation on Android.
Clang-format

dneto0 · 2015-12-09T21:27:00Z

Squashed and pushed onto master.

umar456 added 3 commits November 23, 2015 18:34

Update Validate tests to assignment oriented form

7acf07c

Fix isValid for functions with multiple parameters

d6aaa17

Basic SSA validation.

8033cdb

AWoloszyn reviewed Nov 24, 2015
View reviewed changes

dneto0 reviewed Dec 3, 2015
View reviewed changes

dneto0 reviewed Dec 4, 2015
View reviewed changes

umar456 added 3 commits December 4, 2015 12:27

Fixed OpConstant order in SSA unit tests

be947ef

Remove regex to avoid problems with gcc.

89c4a21

umar456 added 2 commits December 7, 2015 09:55

Add OpName support in diag message and unit tests

ee4c90a

Fix missing label in func;Phi tests;LoopMerge Fix

71b49c4

Addressed feedback

64190ec

dneto0 reviewed Dec 9, 2015
View reviewed changes

dneto0 closed this Dec 9, 2015

antiagainst modified the milestone: SPIR-V validation Jul 27, 2016

dneto0 mentioned this pull request Nov 24, 2017

use-after-free in Instruction::unique_id() #1004

Closed

dneto0 mentioned this pull request Jun 24, 2022

spirv-val: Validate zero product workgroup size #4828

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Basic SSA Validation #27

Basic SSA Validation #27

umar456 commented Nov 23, 2015

AWoloszyn Nov 24, 2015

umar456 Nov 24, 2015

dneto0 Dec 3, 2015

dneto0 commented Dec 3, 2015

dneto0 commented Dec 3, 2015

dneto0 Dec 4, 2015

umar456 Dec 4, 2015

dneto0 Dec 4, 2015

umar456 Dec 5, 2015

umar456 commented Dec 6, 2015

umar456 commented Dec 8, 2015

dneto0 commented Dec 8, 2015

dneto0 commented Dec 8, 2015

umar456 commented Dec 9, 2015

dneto0 Dec 9, 2015

dneto0 commented Dec 9, 2015

dneto0 commented Dec 9, 2015

dneto0 commented Dec 9, 2015

dneto0 commented Dec 9, 2015


		INSTANTIATE_TEST_CASE_P(KernelArgs, Validate, ::testing::ValuesIn(cases));

		string SetOps(string &str, pair<string, bool> param) {

Basic SSA Validation #27

Basic SSA Validation #27

Conversation

umar456 commented Nov 23, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dneto0 commented Dec 3, 2015

dneto0 commented Dec 3, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

umar456 commented Dec 6, 2015

umar456 commented Dec 8, 2015

dneto0 commented Dec 8, 2015

dneto0 commented Dec 8, 2015

umar456 commented Dec 9, 2015

Choose a reason for hiding this comment

dneto0 commented Dec 9, 2015

dneto0 commented Dec 9, 2015

dneto0 commented Dec 9, 2015

dneto0 commented Dec 9, 2015