[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 ↗

(On Diff #527759)

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 ↗

(On Diff #527759)

Integer or Vector of Integers

81 ↗

(On Diff #527759)

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 ↗

(On Diff #527759)

bool isSigned = signess == IntegerType::Signed;

91 ↗

(On Diff #527759)

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 ↗

(On Diff #527759)

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 ↗

(On Diff #527759)

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 ↗

(On Diff #527759)

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 ↗

(On Diff #527759)

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

86–89 ↗

(On Diff #527759)

Thanks, didn't need it anymore.

99 ↗

(On Diff #527759)

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

104–105 ↗

(On Diff #527759)

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