[mlir][spirv] Fix int type declaration duplication when serializing #143108
Conversation
At the MLIR level unsigned integer and signless integers are different types. Indeed when looking up the two types in type definition cache they do not match. Hence when translating a SPIR-V module which contains both usign and signless integers will contain the same type declaration twice (something like OpTypeInt 32 0) which is not permitted in SPIR-V and such generated modules fail validation. This patch solves the problem by mapping unisgned integer types to singless integer types before looking up in the type definition cache. Signed-off-by: Davide Grohmann <[email protected]> Change-Id: I9b5ecc72049d2642308b70acc335b6dc6bf6b5be
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-mlir-spirv @llvm/pr-subscribers-mlir Author: Davide Grohmann (davidegrohmann) ChangesAt the MLIR level unsigned integer and signless integers are different types. Indeed when looking up the two types in type definition cache they do not match. Hence when translating a SPIR-V module which contains both usign and signless integers will contain the same type declaration twice (something like OpTypeInt 32 0) which is not permitted in SPIR-V and such generated modules fail validation. This patch solves the problem by mapping unisgned integer types to singless integer types before looking up in the type definition cache. Change-Id: I9b5ecc72049d2642308b70acc335b6dc6bf6b5be Full diff: https://github.com/llvm/llvm-project/pull/143108.diff 1 Files Affected:
diff --git a/mlir/lib/Target/SPIRV/Serialization/Serializer.cpp b/mlir/lib/Target/SPIRV/Serialization/Serializer.cpp
index 15e06616f4492..c9b2a01091460 100644
--- a/mlir/lib/Target/SPIRV/Serialization/Serializer.cpp
+++ b/mlir/lib/Target/SPIRV/Serialization/Serializer.cpp
@@ -446,6 +446,19 @@ LogicalResult Serializer::processType(Location loc, Type type,
LogicalResult
Serializer::processTypeImpl(Location loc, Type type, uint32_t &typeID,
SetVector<StringRef> &serializationCtx) {
+
+ // Map unisgned integer types to singless integer types
+ // This is needed otherwise the generated spirv assembly will contain
+ // twice a type declaration (like OpTypeInt 32 0) which is no permitted and
+ // such module could no pass validation. Indeed at MLIR level the two types
+ // are different and lookup in the cache below fails.
+ // Note: This convertion needs to happen here before the type is looked up in
+ // the cache
+ if (type.isUnsignedInteger()) {
+ type = IntegerType::get(loc->getContext(), type.getIntOrFloatBitWidth(),
+ IntegerType::SignednessSemantics::Signless);
+ }
+
typeID = getTypeID(type);
if (typeID)
return success();
|
Also add capabilities and entry point to allow the spirv assembly generated from the constants.mlir test to pass validation Signed-off-by: Davide Grohmann <[email protected]> Change-Id: I22df9051de66a3dcb11d70bb95d06801aea62942
There is already a test checking that a mix of unsigned/signless ints in the same SPIRV module can be serialized/deserialized to/from MLIR and SPIRV (see mlir/test/Target/SPIRV/constant.mlir). The test pass since the deserializer does not fail on the invalid SPIRV code, validating the spriv assembly from that test fails with the following without the code fix in this patch: The only way to test this properly is to add a validation step on the produced SPIRV assembly, but that cannot be achieve easily at the moment. SPIR-V assembly from constants.mlir before the change (marked double declaration by hand): After the patch: |
I thought we have some tests that use spir-v when present: Could we do something similar in mlir spirv serialization? If this is not straightforward, validating by hand is also fine IMO. |
SPIR-V validation is run if spirv-tools are available otherwise skipped. Signed-off-by: Davide Grohmann <[email protected]> Change-Id: Id0af7c983c4bd6596a4837812f22ced3c7616177
Thanks for the suggestion I have added that to the test. I have also validated manually the SPIR-V module produced by that test and it passes locally for me. |
Have you checked that the spirv tools invocation works as expected and fails to validate without your fix? I haven't tried it myself, so I'm not sure if it has to be enabled somewhere in cmake etc. |
I was hoping that to work out of the box since there are also other tests in the same folder using the same spirv-tools configuration, for example: In any case I have tried to make it work locally without success. I have built everything enabling the SPIRV target and enable the SPIRV tests by adding Full command: Any ideas about what might I be missing? In any case I have manually tested that the command |
I'd try to use the git history and see how this was initially plumbed through in clang tests. Once we figure this out, it will be very valuable in other tests that deal with (de)serialization. If some cmake configuration is required, we can document it under https://mlir.llvm.org/getting_started/TestingGuide/ .
This looks unrelated to me. |
|
See https://github.com/llvm/llvm-project/pull/73044/files . I think we are missing a local lit config entry. |
Thanks for the link I'll have a look |
Including the SPIRV target is needed otherwise the following error path triggers in |
Ah, I understand. Supporting spirv-tools in tests and building the spirv backend can be decoupled in the future -- fine to leave as-is for now though. |
Also fix validation failures caused by a rebase Signed-off-by: Davide Grohmann <[email protected]> Change-Id: I221881875c77a30a4df4bd9b15288ca10e9c8e9c
It works for me locally now with the latest commit |
|
@kuhar is there anything more that is required for this PR to be merged? |
|
@davidegrohmann Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
Merged. @davidegrohmann could you submit a follow up PR and update the mlir documentation to explain how to run tests that use spirv tools and also decouple the cmake flag for spirv tools from the flag that enables the spirv backend? |
davidegrohmann
deleted the
fix-unsign-signless-type-serialization-duplication
branch
…lvm#143108) At the MLIR level unsigned integer and signless integers are different types. Indeed when looking up the two types in type definition cache they do not match. Hence when translating a SPIR-V module which contains both usign and signless integers will contain the same type declaration twice (something like OpTypeInt 32 0) which is not permitted in SPIR-V and such generated modules fail validation. This patch solves the problem by mapping unisgned integer types to singless integer types before looking up in the type definition cache. --------- Signed-off-by: Davide Grohmann <[email protected]>
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/130/builds/13792 Here is the relevant piece of the build log for the reference |
| @@ -0,0 +1,7 @@ | |||
| if not "SPIRV" in config.root.targets: | |||
| config.unsupported = True | |||
There was a problem hiding this comment.
Why do we need the SPIRV backend to run MLIR unit-tests?
There was a problem hiding this comment.
Please see this comment: #143108 (comment)
There was a problem hiding this comment.
I missed this -- if this disables all mlir tests, we should revert the PR
There was a problem hiding this comment.
This was partially fixed in #144685 but now disables all spirv tests...
@davidegrohmann can you prepare a fix by EoD? Otherwise we should revert this.
There was a problem hiding this comment.
Sorry I miss your comment it was already EoD for me. Thanks for reverting the patch. I will prepare a new PR without the problematic code.
…lvm#143108) At the MLIR level unsigned integer and signless integers are different types. Indeed when looking up the two types in type definition cache they do not match. Hence when translating a SPIR-V module which contains both usign and signless integers will contain the same type declaration twice (something like OpTypeInt 32 0) which is not permitted in SPIR-V and such generated modules fail validation. This patch solves the problem by mapping unisgned integer types to singless integer types before looking up in the type definition cache. --------- Signed-off-by: Davide Grohmann <[email protected]>
… when serializing" and follow up commits (#144773) This reverts the following PRs: * llvm/llvm-project#143108 * llvm/llvm-project#144538 * llvm/llvm-project#144685 Reverting because this disabled tests when building without the llvm spirv backend enabled.
At the MLIR level unsigned integer and signless integers are different types. Indeed when looking up the two types in type definition cache they do not match.
Hence when translating a SPIR-V module which contains both usign and signless integers will contain the same type declaration twice (something like OpTypeInt 32 0) which is not permitted in SPIR-V and such generated modules fail validation.
This patch solves the problem by mapping unisgned integer types to singless integer types before looking up in the type definition cache.