[mlir][spirv] Change numeric constant's bit-extension decision to be based on type

[mshahneo]

Summary

Integer constants with bit width less than a word (e.g., i8, i16)
should be bit extended based on its type to be SPIR-V spec-compliant.
Previously, the decision was based on the most significant bit of the
value which ignores the signless semantics and causes problems when
interfacing with SPIR-V tools.

Dealing with numeric literals: the SPIR-V spec says, "If a numeric
type’s bit width is less than 32-bits, the value appears in the
low-order bits of the word, and the high-order bits must be 0 for
a floating-point type or integer type with Signedness of 0, or sign
extended for an integer type with a Signedness of 1 (similarly for the
remaining bits of widths larger than 32 bits but not a multiple of 32
bits)."

Therefore, signless integers (e.g., i8, i16) and unsigned integers
should be 0-extended, and signed integers (e.g., si8, si16) should be
sign-extended.

[kuhar]

Would it be possible to write a test that shows the difference between the existing code and this change?

[antiagainst]

In D151767#4384876, @kuhar wrote:

Would it be possible to write a test that shows the difference between the existing code and this change?

+1 You can look at the unittests/Dialect/SPIRV/SerializationTest.cpp and try to add some tests there.

[mshahneo]

Add test cases.

[mshahneo]

In D151767#4384876, @kuhar wrote:

Would it be possible to write a test that shows the difference between the existing code and this change?

Absolutely, added some tests.

[mshahneo]

In D151767#4385539, @antiagainst wrote:

In D151767#4384876, @kuhar wrote:

Would it be possible to write a test that shows the difference between the existing code and this change?

+1 You can look at the unittests/Dialect/SPIRV/SerializationTest.cpp and try to add some tests there.

Thank you so much @antiagainst!

[kuhar]

mlir/test/Target/SPIRV/constant.mlir

276

If we return the constants here then we won't have to worry about these two getting rearranged or DCE'd.

mlir/unittests/Dialect/SPIRV/SerializationTest.cpp

79

Integer or Vector of Integers

81

Since the point of the test is to exercise sign/zero extension, can we change val to APInt so that we don't have to worry about how the conversions work on the C++ level when we pass an int parameter?

86–89

bool isSigned = signess == IntegerType::Signed;

91

nit: we don't need llvm:: in front of casts -- the existing ones in this file it's just a side effect of a recent automated refactoring

99

Are vector constants used by the tests? I don't see any. If that's the case, should we remove vector support altogether?

104–105

Do we want to have two distinct ways of creating integer attributed here? Why don't we use the same code as above?

[mshahneo]

Addressed the review comments.

[mshahneo]

Thank you so much for the suggestions. Addressed them.

Please let me know what yo think.

Also, if it's accepted, could you please help me merge it, I don't have merge permission.

mlir/test/Target/SPIRV/constant.mlir

276

I actually thought about that, hence added the two add instructions just so they don't get DCE'd.

But you are right, in the future if the DCE becomes more aggressive it may remove these.

Made two test cases.

mlir/unittests/Dialect/SPIRV/SerializationTest.cpp

81

Good point, thank you so much, changed the val to APInt.

86–89

Thanks, didn't need it anymore.

99

Vector constants are not used by this test, just kept it in case of future use.

104–105

Good point, made them to both use the similar API. Second one is just a wrapper.

[kuhar]

LGTM

[kuhar]

Done, submitted in https://reviews.llvm.org/rG2dab551d649cb57127f7b3bea183f069883d76e4.

BTW, I looked up your commit history and with 4 commits already in you should be able to request commit access: https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access

[mshahneo]

In D151767#4395971, @kuhar wrote:

Done, submitted in https://reviews.llvm.org/rG2dab551d649cb57127f7b3bea183f069883d76e4.

BTW, I looked up your commit history and with 4 commits already in you should be able to request commit access: https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access

Thank you so much :).
Will ask for commit access.

mlir/lib/Target/SPIRV/Serialization/Serializer.cpp

uint32_t Serializer::prepareConstantInt(Location loc, IntegerAttr intAttr,
   uint32_t typeID = 0;
   if (failed(processType(loc, intAttr.getType(), typeID))) {
     return 0;
   }
 
   auto resultID = getNextID();
   APInt value = intAttr.getValue();
   unsigned bitwidth = value.getBitWidth();
-  bool isSigned = value.isSignedIntN(bitwidth);
-
+  bool isSigned = intAttr.getType().isSignedInteger();
   auto opcode =
       isSpec ? spirv::Opcode::OpSpecConstant : spirv::Opcode::OpConstant;
 
   switch (bitwidth) {
     // According to SPIR-V spec, "When the type's bit width is less than
     // 32-bits, the literal's value appears in the low-order bits of the word,
     // and the high-order bits must be 0 for a floating-point type, or 0 for an
     // integer type with Signedness of 0, or sign extended when Signedness

mlir/test/Target/SPIRV/constant.mlir

spirv.module Logical GLSL450 requires #spirv.vce<v1.0, [Shader], []> {
   }
 
   // CHECK-LABEL: @multi_dimensions_splat_const
   spirv.func @multi_dimensions_splat_const() -> (!spirv.array<2 x !spirv.array<2 x !spirv.array<3 x i32, stride=4>, stride=12>, stride=24>) "None" {
     // CHECK: spirv.Constant {{\[}}{{\[}}[1 : i32, 1 : i32, 1 : i32], [1 : i32, 1 : i32, 1 : i32]], {{\[}}[1 : i32, 1 : i32, 1 : i32], [1 : i32, 1 : i32, 1 : i32]]] : !spirv.array<2 x !spirv.array<2 x !spirv.array<3 x i32, stride=4>, stride=12>, stride=24>
     %0 = spirv.Constant dense<1> : tensor<2x2x3xi32> : !spirv.array<2 x !spirv.array<2 x !spirv.array<3 x i32, stride=4>, stride=12>, stride=24>
     spirv.ReturnValue %0 : !spirv.array<2 x !spirv.array<2 x !spirv.array<3 x i32, stride=4>, stride=12>, stride=24>
   }
+
+  // CHECK-LABEL: @signless_int_const_bit_extension
+  spirv.func @signless_int_const_bit_extension() -> (i16) "None" {
+    // CHECK: spirv.Constant -1 : i16
+    %signless_minus_one = spirv.Constant -1 : i16
+    spirv.ReturnValue %signless_minus_one : i16
+  }
+  // CHECK-LABEL: @signed_int_const_bit_extension
+  spirv.func @signed_int_const_bit_extension() -> (si16) "None" {
+    // CHECK: spirv.Constant -1 : si16
+    %signed_minus_one = spirv.Constant -1 : si16
+    spirv.ReturnValue %signed_minus_one : si16
+  }
 }

mlir/unittests/Dialect/SPIRV/SerializationTest.cpp

protected:
   spirv::GlobalVariableOp addGlobalVar(Type type, llvm::StringRef name) {
     OpBuilder builder(module->getRegion());
     auto ptrType = spirv::PointerType::get(type, spirv::StorageClass::Uniform);
     return builder.create<spirv::GlobalVariableOp>(
         UnknownLoc::get(&context), TypeAttr::get(ptrType),
         builder.getStringAttr(name), nullptr);
   }
 
+  // Inserts an Integer or a Vector of Integers constant of value 'val'.
+  spirv::ConstantOp AddConstInt(Type type, APInt val) {
+    OpBuilder builder(module->getRegion());
+    auto loc = UnknownLoc::get(&context);
+
+    if (auto intType = dyn_cast<IntegerType>(type)) {
+      return builder.create<spirv::ConstantOp>(
+          loc, type, builder.getIntegerAttr(type, val));
+    }
+    if (auto vectorType = dyn_cast<VectorType>(type)) {
+      Type elemType = vectorType.getElementType();
+      if (auto intType = dyn_cast<IntegerType>(elemType)) {
+        return builder.create<spirv::ConstantOp>(
+            loc, type,
+            DenseElementsAttr::get(vectorType,
+                                   IntegerAttr::get(elemType, val).getValue()));
+      }
+    }
+    llvm_unreachable("unimplemented types for AddConstInt()");
+  }
+
   /// Handles a SPIR-V instruction with the given `opcode` and `operand`.
   /// Returns true to interrupt.
   using HandleFn = llvm::function_ref<bool(spirv::Opcode opcode,
                                            ArrayRef<uint32_t> operands)>;
 
   /// Returns true if we can find a matching instruction in the SPIR-V blob.
   bool scanInstruction(HandleFn handleFn) {
     auto binarySize = binary.size();
     auto *begin = binary.begin();
     auto currOffset = spirv::kHeaderWordCount;
 
     while (currOffset < binarySize) {
       auto wordCount = binary[currOffset] >> 16;
       if (!wordCount || (currOffset + wordCount > binarySize))
if (opcode == spirv::Opcode::OpDecorate && operands.size() == 2 &&
         operands[1] == static_cast<uint32_t>(spirv::Decoration::Block))
       ++count;
     return false;
   };
   ASSERT_FALSE(scanInstruction(countBlockDecoration));
   EXPECT_EQ(count, 1u);
 }
 
+TEST_F(SerializationTest, SignlessVsSignedIntegerConstantBitExtension) {
+
+  auto signlessInt16Type =
+      IntegerType::get(&context, 16, IntegerType::Signless);
+  auto signedInt16Type = IntegerType::get(&context, 16, IntegerType::Signed);
+  // Check the bit extension of same value under different signedness semantics.
+  APInt signlessIntConstVal(signlessInt16Type.getWidth(), -1,
+                            signlessInt16Type.getSignedness());
+  APInt signedIntConstVal(signedInt16Type.getWidth(), -1,
+                          signedInt16Type.getSignedness());
+
+  AddConstInt(signlessInt16Type, signlessIntConstVal);
+  AddConstInt(signedInt16Type, signedIntConstVal);
+  ASSERT_TRUE(succeeded(spirv::serialize(module.get(), binary)));
+
+  auto hasSignlessVal = [&](spirv::Opcode opcode, ArrayRef<uint32_t> operands) {
+    return opcode == spirv::Opcode::OpConstant && operands.size() == 3 &&
+           operands[2] == 65535;
+  };
+  EXPECT_TRUE(scanInstruction(hasSignlessVal));
+
+  auto hasSignedVal = [&](spirv::Opcode opcode, ArrayRef<uint32_t> operands) {
+    return opcode == spirv::Opcode::OpConstant && operands.size() == 3 &&
+           operands[2] == 4294967295;
+  };
+  EXPECT_TRUE(scanInstruction(hasSignedVal));
+}
+
 TEST_F(SerializationTest, ContainsSymbolName) {
   auto structType = getFloatStructType();
   addGlobalVar(structType, "var0");
 
   spirv::SerializationOptions options;
   options.emitSymbolName = true;
   ASSERT_TRUE(succeeded(spirv::serialize(module.get(), binary, options)));