Extension allows multiple same OpTypePointer types #783

atgoo · 2017-08-29T20:34:31Z

SPV_KHR_variable_pointers allows OpTypePointer to declare multiple
identical types

SPV_KHR_variable_pointers allows OpTypePointer to declare multiple identical types if they have different declarations. KhronosGroup#781

dneto0

Thanks for the quick turnaround.
I think it's not quite right in its internals, and I think some of the tests are incorrect.

dneto0 · 2017-08-29T22:37:34Z

test/val/val_type_unique_test.cpp

@@ -238,4 +238,57 @@ OpMemoryModel Physical32 OpenCL
              Not(HasSubstr(GetErrorString(SpvOpTypeVoid))));
 }

+TEST_F(ValidateTypeUnique, PointerTypesSameArrayStrideNoExtension) {


The test name is misleading to me: The array strides here are different, but the test name is ....SameArrayStride...

dneto0 · 2017-08-29T22:39:20Z

source/val/validation_state.cpp

+      const auto result = decoration_params.emplace(decoration.dec_type(),
+                                                    &decoration.params());
+      (void)result;
+      assert(result.second);


I don't think you should assert here? There's nothing wrong with redundant decorations, except that it wastes space.
I don't think the storage behind id_decorations has unique'd it.

It's worth saying you're assuming that a single decoration enum can only be set once per target ID.
It think that's true currently, and is likely to be true always. But I don't recall a SPIR-V validation rule about that. (Perhaps an oversight.)

I will assume that decorations are not unique, but only use the last one.

dneto0 · 2017-08-29T22:39:34Z

source/val/validation_state.cpp

@@ -425,6 +426,22 @@ bool ValidationState_t::RegisterUniqueTypeDeclaration(
    key.insert(key.end(), inst.words + words_begin, inst.words + words_end);
  }

+  if (inst.opcode == SpvOpTypePointer &&


Better to have a comment describing the rule you're enforcing.

dneto0 · 2017-08-29T22:46:29Z

test/val/val_type_unique_test.cpp

+  CompileSuccessfully(str.c_str());
+  ASSERT_EQ(SPV_SUCCESS, ValidateInstructions());
+  EXPECT_THAT(getDiagnosticString(),
+              HasSubstr(GetErrorString(SpvOpTypePointer)));


I'm confused. The validation succeeds, but generates an error string?

Does this test even pass?

It logs a warning but returns SPV_SUCCESS.

dneto0 · 2017-08-29T22:47:28Z

source/validate_type_unique.cpp

@@ -49,7 +49,7 @@ spv_result_t TypeUniquePass(ValidationState_t& _,
      // return _.diag(SPV_ERROR_INVALID_DATA)
      return _.diag(SPV_SUCCESS)
          << "Duplicate non-aggregate type declarations are not allowed."
-          << " Opcode: " << inst->opcode;
+          << " Opcode: " << spvOpcodeString(SpvOp(inst->opcode));


This is an improvement. But in actual practice I've found having the ID generated by this instruction is easier to work with. It pinpoints the exact instruction at fault.

dneto0 · 2017-08-29T22:48:08Z

test/val/val_type_unique_test.cpp

+OpCapability Shader
+OpCapability Linkage
+OpMemoryModel Logical GLSL450
+OpDecorate %ptr1 ArrayStride 4


Or maybe you just have a typo here?

atgoo

Ready for review.

atgoo · 2017-08-30T14:32:41Z

source/val/validation_state.cpp

+      const auto result = decoration_params.emplace(decoration.dec_type(),
+                                                    &decoration.params());
+      (void)result;
+      assert(result.second);


I will assume that decorations are not unique, but only use the last one.

atgoo · 2017-08-30T14:32:46Z

source/val/validation_state.cpp

@@ -425,6 +426,22 @@ bool ValidationState_t::RegisterUniqueTypeDeclaration(
    key.insert(key.end(), inst.words + words_begin, inst.words + words_end);
  }

+  if (inst.opcode == SpvOpTypePointer &&


atgoo · 2017-08-30T14:35:27Z

source/validate_type_unique.cpp

@@ -49,7 +49,7 @@ spv_result_t TypeUniquePass(ValidationState_t& _,
      // return _.diag(SPV_ERROR_INVALID_DATA)
      return _.diag(SPV_SUCCESS)
          << "Duplicate non-aggregate type declarations are not allowed."
-          << " Opcode: " << inst->opcode;
+          << " Opcode: " << spvOpcodeString(SpvOp(inst->opcode));


atgoo · 2017-08-30T14:35:51Z

test/val/val_type_unique_test.cpp

@@ -238,4 +238,57 @@ OpMemoryModel Physical32 OpenCL
              Not(HasSubstr(GetErrorString(SpvOpTypeVoid))));
 }

+TEST_F(ValidateTypeUnique, PointerTypesSameArrayStrideNoExtension) {


atgoo · 2017-08-30T14:37:12Z

test/val/val_type_unique_test.cpp

+  CompileSuccessfully(str.c_str());
+  ASSERT_EQ(SPV_SUCCESS, ValidateInstructions());
+  EXPECT_THAT(getDiagnosticString(),
+              HasSubstr(GetErrorString(SpvOpTypePointer)));


It logs a warning but returns SPV_SUCCESS.

dneto0

The rule should be more relaxed than what you implemented.

dneto0 · 2017-08-30T21:01:52Z

source/val/validation_state.cpp

+    for (const auto& kv : decoration_params) {
+      // Append decoration data to the key, sorted by decoration type.
+      // Use magic number to separate unrelated chunks of data.
+      const uint32_t kMagicSeparator = 0xffff9296;


Low level comment: This magic number mechanism is fragile. Better to push a kv.second.size() then its contents.

(But really we should not care about duplication of decorations.)

dneto0 · 2017-08-30T21:07:16Z

source/val/validation_state.cpp

@@ -425,6 +426,25 @@ bool ValidationState_t::RegisterUniqueTypeDeclaration(
    key.insert(key.end(), inst.words + words_begin, inst.words + words_end);
  }

+  // If module has extension SPV_KHR_variable_pointers, then OpTypePointer
+  // is allowed to create identical types so long as their decorations differ.


Looking again, this is not the rule.
The SPV_KHR_variable_pointers extension says

Pointer types are also allowed to have multiple id's for the same opcode and operands, to allow for differing ArrayStride Array Stride decoration values.

Also, in that paragraph, "non-aggregate types" will then generally be "non-aggregate non-pointer types."

From 2.8 in the SPIR-V spec:

It is valid to declare multiple aggregate type s having the same opcode and operands. This is to allow multiple instances of aggregate types with the same structure to be decorated differently. (Different decorations are not required; two different aggregate type ids are allowed to have identical declarations and decorations, and will still be two different types.)

That means the validator should allow distinct OpTypePointer with the same operands, and with the same decorations.

So I think all this new code boils down to:

if (inst.opcode == SpvOpTypePointer && HasExtension....) return true;

Even better: put the check up in the callee, along with the aggregate types. That is, modify TypeUniquePass to do the pointer-and-has-extension check.

atgoo · 2017-08-31T17:34:27Z

Ready for review.

dneto0

Aside from being correct, this is even cleaner.
LGTM, and thanks.

dneto0 · 2017-09-01T14:14:51Z

Hmm. Except that the commit message was inaccurate. I'll fix it on the rebase.

dneto0 · 2017-09-01T14:17:18Z

Rebased, squashed, and pushed into master as 725284c

Extension allows multiple same OpTypePointer types

6930bdc

SPV_KHR_variable_pointers allows OpTypePointer to declare multiple identical types if they have different declarations. KhronosGroup#781

atgoo requested review from antiagainst and dneto0 August 29, 2017 20:34

dneto0 requested changes Aug 29, 2017

View reviewed changes

dneto0 mentioned this pull request Aug 30, 2017

Reduce duplication of pointer types google/clspv#52

Closed

Review changes

e49fc32

atgoo commented Aug 30, 2017

View reviewed changes

dneto0 requested changes Aug 30, 2017

View reviewed changes

Andrey Tuganov added 2 commits August 30, 2017 17:43

Review changes 2

433456d

remove <map>

437ac6a

dneto0 approved these changes Sep 1, 2017

View reviewed changes

dneto0 closed this Sep 1, 2017

dneto0 mentioned this pull request Sep 1, 2017

Validator: Should allow OpTypePointer with same pointee type and storage class #781

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extension allows multiple same OpTypePointer types #783

Extension allows multiple same OpTypePointer types #783

atgoo commented Aug 29, 2017 •

edited

dneto0 left a comment

dneto0 Aug 29, 2017

atgoo Aug 30, 2017

dneto0 Aug 29, 2017

dneto0 Aug 29, 2017

atgoo Aug 30, 2017

dneto0 Aug 29, 2017

atgoo Aug 30, 2017

dneto0 Aug 29, 2017

atgoo Aug 30, 2017

dneto0 Aug 29, 2017

atgoo Aug 30, 2017

dneto0 Aug 29, 2017

atgoo left a comment

atgoo Aug 30, 2017

atgoo Aug 30, 2017

atgoo Aug 30, 2017

atgoo Aug 30, 2017

atgoo Aug 30, 2017

dneto0 left a comment

dneto0 Aug 30, 2017

dneto0 Aug 30, 2017

dneto0 Aug 30, 2017

atgoo commented Aug 31, 2017

dneto0 left a comment

dneto0 commented Sep 1, 2017

dneto0 commented Sep 1, 2017

Extension allows multiple same OpTypePointer types #783

Extension allows multiple same OpTypePointer types #783

Conversation

atgoo commented Aug 29, 2017 • edited

dneto0 left a comment

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

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

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

Choose a reason for hiding this comment

atgoo left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dneto0 left a comment

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

atgoo commented Aug 31, 2017

dneto0 left a comment

Choose a reason for hiding this comment

dneto0 commented Sep 1, 2017

dneto0 commented Sep 1, 2017

atgoo commented Aug 29, 2017 •

edited