Repository: KhronosGroup/SPIRV-Tools (language: python)

Issue:
Inconsistent behaviour related to OpCompositeConstant
**TL;DR:** The constant manager does not accept some integer vector
constants that the validator considers valid and accepts other ones
that should not be considered valid.

----

Given the following instructions:

```
%1 = OpTypeInt 32 0
%2 = OpTypeInt 32 1
%3 = OpConstant %1 1
%4 = OpConstant %2 1
%5 = OpTypeVector %1 4
%6 = OpConstantComposite %5 %4 %4 %4 %4
%7 = OpConstantComposite %5 %3 %4 %3 %4
```

The constant manager does not record vector %7 as being a constant.
In fact, the comment above the CreateConstant function declaration
in constants.h says:

"
  When creating VectorType Constant instance, the components must be
  scalars of the same type, either Bool, Integer or Float.
"
which is consistent with the behaviour, because %3 and %4 have
different types (%3 is an unsigned integer, %4 is signed). Weirdly,
%6 is recognised, even if the type of the vector is unsigned int
and the type of the components is signed int.

However, the validator does not complain about %7, which can be
used as a vector of unsigned integers and works as expected.

Adding the following lines:

```
%8 = OpTypeFloat 32
%9 = OpConstant %8 0
%10 = OpConstantComposite %5 %9 %9 %9 %9
```

the constant manager accepts %10 as a vector constant while the
validator (rightly) complains that the component type and the
vector type are not compatible.

This is probably because, in constants.cpp, the first element type
rather than the component type of the vector is checked against to
verify that all types are the same.

From lines 269-275 of constants.cpp:

```c++
const auto* component_type = components.front()->type();
if (!std::all_of(components.begin(), components.end(),
                 [&component_type](const Constant* c) {
                   if (c->type() == component_type) return true;
                   return false;
                 }))
  return nullptr;
```

----
- %10 being recognised seems an error by the constant manager
- %7 not being accepted by the constant manager but accepted by the
  validator shows an inconsistency. Which one is right?
 
The specification for OpCompositeConstant says:

"
Result Type must be a composite type, whose top-level
members/elements/components/columns have the same
type as the types of the Constituents.
"


Discussion / hints:
@stefanomil Nice TL;DR, but I'd put that at the start of your message :)  (You can edit.)

@alan-baker / @s-perron - I would value your input here.

Regarding "%10 being recognised seems an error by the constant manager": my view on an invalid module is that we don't care too much what the constant manager does: I think it's reasonable to only care about the constant manager's behaviour on SPIR-V that the validator accepts.

Regarding "%7 not being accepted by the constant manager but accepted by the validator shows an inconsistency. Which one is right?" - the validator is corect that this is valid.  Whether the constant manager is wrong depends on the goals of the constant manager.  Is it supposed to manage all constants, or is it deliberately restricted to constants that are in some sense "nice"?  (i.e. that do not mix sign in the way they are constructed)


>  I think it's reasonable to only care about the constant manager's behaviour on SPIR-V that the validator accepts.

I agree with that.

>  Is it supposed to manage all constants, or is it deliberately restricted to constants that are in some sense "nice"?

It should manage all constants.  However, I am surprised that %7 is legal.  The spec says "Result Type must be a composite type, whose top-level members/elements/components/columns have the same type as the types of the Constituents."  In this case, the member type does not match the constituent type.  However, I am not an expert on the spec.  I'll defer to Alan on that.
Thanks @s-perron.  @alan-baker what do you think re the legality or not of %7?
I would have thought the validator would catch this. At first glance the code is there to cover this case. I do agree it should be invalid.
@stefanomil Can you upload a spirv binary that exhitits the problem and confirm that spirv-val (when compiled from the latest source) indeed doesn't complain about it?
[Here it is](https://github.com/KhronosGroup/SPIRV-Tools/files/4987432/shader.txt). It still does not complain. I changed the extension to .txt so that I could upload the file, but it actually is the binary. Running spirv-dis on it gives:

```
; SPIR-V
; Version: 1.5
; Generator: Khronos SPIR-V Tools Assembler; 0
; Bound: 13
; Schema: 0
               OpCapability Shader
          %1 = OpExtInstImport "GLSL.std.450"
               OpMemoryModel Logical GLSL450
               OpEntryPoint Fragment %main "main"
               OpExecutionMode %main OriginUpperLeft
               OpSource ESSL 310
               OpName %main "main"
       %void = OpTypeVoid
       %uint = OpTypeInt 32 0
        %int = OpTypeInt 32 1
     %uint_1 = OpConstant %uint 1
      %int_1 = OpConstant %int 1
     %v4uint = OpTypeVector %uint 4
          %9 = OpConstantComposite %v4uint %uint_1 %uint_1 %uint_1 %uint_1
         %10 = OpConstantComposite %v4uint %uint_1 %int_1 %uint_1 %int_1
         %11 = OpTypeFunction %void
       %main = OpFunction %void None %11
         %12 = OpLabel
               OpReturn
               OpFunctionEnd
```


Fix patch:
diff --git a/source/val/validate_constants.cpp b/source/val/validate_constants.cpp
index a8ee5a6b1e..006e504c0c 100644
--- a/source/val/validate_constants.cpp
+++ b/source/val/validate_constants.cpp
@@ -76,7 +76,7 @@ spv_result_t ValidateConstantComposite(ValidationState_t& _,
         }
         const auto constituent_result_type = _.FindDef(constituent->type_id());
         if (!constituent_result_type ||
-            component_type->opcode() != constituent_result_type->opcode()) {
+            component_type->id() != constituent_result_type->id()) {
           return _.diag(SPV_ERROR_INVALID_ID, inst)
                  << opcode_name << " Constituent <id> "
                  << _.getIdName(constituent_id)


Test patch:
diff --git a/test/val/val_constants_test.cpp b/test/val/val_constants_test.cpp
index 301539d98f..47278777dc 100644
--- a/test/val/val_constants_test.cpp
+++ b/test/val/val_constants_test.cpp
@@ -478,6 +478,22 @@ OpName %ptr "ptr"
                         "a null value"));
 }
 
+TEST_F(ValidateConstant, VectorMismatchedConstituents) {
+  std::string spirv = kShaderPreamble kBasicTypes R"(
+%int = OpTypeInt 32 1
+%int_0 = OpConstantNull %int
+%const_vector = OpConstantComposite %uint2 %uint_0 %int_0
+)";
+
+  CompileSuccessfully(spirv);
+  EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions());
+  EXPECT_THAT(
+      getDiagnosticString(),
+      HasSubstr(
+          "OpConstantComposite Constituent <id> '13[%13]'s type "
+          "does not match Result Type <id> '3[%v2uint]'s vector element type"));
+}
+
 }  // namespace
 }  // namespace val
 }  // namespace spvtools
diff --git a/test/val/val_extension_spv_khr_integer_dot_product_test.cpp b/test/val/val_extension_spv_khr_integer_dot_product_test.cpp
index 041751d944..5b3a309612 100644
--- a/test/val/val_extension_spv_khr_integer_dot_product_test.cpp
+++ b/test/val/val_extension_spv_khr_integer_dot_product_test.cpp
@@ -128,7 +128,7 @@ std::string AssemblyForCase(const Case& c) {
          %char_0 = OpConstant %char 0
          %char_1 = OpConstant %char 1
 
-         %v4uchar_0 = OpConstantComposite %v4uchar %uchar_0 %uint_0 %uchar_0 %uchar_0
+         %v4uchar_0 = OpConstantComposite %v4uchar %uchar_0 %uchar_0 %uchar_0 %uchar_0
          %v4uchar_1 = OpConstantComposite %v4uchar %uchar_1 %uchar_1 %uchar_1 %uchar_1
          %v4char_0 = OpConstantComposite %v4char %char_0 %char_0 %char_0 %char_0
          %v4char_1 = OpConstantComposite %v4char %char_1 %char_1 %char_1 %char_1