[mlir][spirv] Truncate Literal String size at max number words #142916

davidegrohmann · 2025-06-05T07:59:16Z

If not truncated the SPIRV serialization would not fail but instead produce an invalid SPIR-V module.

If not truncated the SPIRV serialization would not fail but instead produce an invalid SPIR-V module. Change-Id: I54c4e54d6ad081861b524d4ae236a1e5080b88c4 Signed-off-by: Davide Grohmann <davide.grohmann@arm.com>

github-actions · 2025-06-05T07:59:38Z

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 @ followed by their GitHub username.

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.

llvmbot · 2025-06-05T08:00:11Z

@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-mlir-spirv

Author: Davide Grohmann (davidegrohmann)

Changes

If not truncated the SPIRV serialization would not fail but instead produce an invalid SPIR-V module.

Full diff: https://github.com/llvm/llvm-project/pull/142916.diff

2 Files Affected:

(modified) mlir/include/mlir/Target/SPIRV/SPIRVBinaryUtils.h (+6)
(modified) mlir/lib/Target/SPIRV/SPIRVBinaryUtils.cpp (+15-1)

diff --git a/mlir/include/mlir/Target/SPIRV/SPIRVBinaryUtils.h b/mlir/include/mlir/Target/SPIRV/SPIRVBinaryUtils.h
index e46a576f1d48e..d3847ae3d3bb2 100644
--- a/mlir/include/mlir/Target/SPIRV/SPIRVBinaryUtils.h
+++ b/mlir/include/mlir/Target/SPIRV/SPIRVBinaryUtils.h
@@ -30,6 +30,12 @@ constexpr uint32_t kMagicNumber = 0x07230203;
 /// The serializer tool ID registered to the Khronos Group
 constexpr uint32_t kGeneratorNumber = 22;
 
+// Max number of words
+constexpr uint32_t kMaxWordCount = 65535;
+
+// Max number of words for literal
+constexpr uint32_t kMaxLiteralWordCount = kMaxWordCount - 3;
+
 /// Appends a SPRI-V module header to `header` with the given `version` and
 /// `idBound`.
 void appendModuleHeader(SmallVectorImpl<uint32_t> &header,
diff --git a/mlir/lib/Target/SPIRV/SPIRVBinaryUtils.cpp b/mlir/lib/Target/SPIRV/SPIRVBinaryUtils.cpp
index 31205d8f408f1..4d4d67a012ae1 100644
--- a/mlir/lib/Target/SPIRV/SPIRVBinaryUtils.cpp
+++ b/mlir/lib/Target/SPIRV/SPIRVBinaryUtils.cpp
@@ -13,6 +13,9 @@
 #include "mlir/Target/SPIRV/SPIRVBinaryUtils.h"
 #include "mlir/Dialect/SPIRV/IR/SPIRVTypes.h"
 #include "llvm/Config/llvm-config.h" // for LLVM_VERSION_MAJOR
+#include "llvm/Support/Debug.h"
+
+#define DEBUG_TYPE "spirv-binary-utils"
 
 using namespace mlir;
 
@@ -68,7 +71,18 @@ void spirv::encodeStringLiteralInto(SmallVectorImpl<uint32_t> &binary,
                                     StringRef literal) {
   // We need to encode the literal and the null termination.
   auto encodingSize = literal.size() / 4 + 1;
+  auto sizeOfDataToCopy = literal.size();
+  if (encodingSize >= kMaxLiteralWordCount) {
+    // reserve one word for the null termination
+    encodingSize = kMaxLiteralWordCount - 1;
+    // do not override the last word (null termination) when copying
+    sizeOfDataToCopy = (encodingSize - 1) * 4;
+    LLVM_DEBUG(llvm::dbgs() << "Truncating string literal to max size ("
+                            << std::to_string(kMaxLiteralWordCount - 1)
+                            << "): " << literal << "\n");
+  }
   auto bufferStartSize = binary.size();
   binary.resize(bufferStartSize + encodingSize, 0);
-  std::memcpy(binary.data() + bufferStartSize, literal.data(), literal.size());
+  std::memcpy(binary.data() + bufferStartSize, literal.data(),
+              sizeOfDataToCopy);
 }

IgWod-IMG

Thank you for your contribution!

If possible, could you please try to add a test to exercise this change? I believe a target test should do the trick (see mlir/test/Target/SPIRV). Those tests serialize given MLIR into SPIR-V and then deserialize the SPIR-V back to MLIR. So, I would hope you could see a truncation after the roundtrip.

mlir/lib/Target/SPIRV/SPIRVBinaryUtils.cpp

davidegrohmann · 2025-06-05T11:53:22Z

Thank you for your contribution!

If possible, could you please try to add a test to exercise this change? I believe a target test should do the trick (see mlir/test/Target/SPIRV). Those tests serialize given MLIR into SPIR-V and then deserialize the SPIR-V back to MLIR. So, I would hope you could see a truncation after the roundtrip.

It has been tricky to add a good test for this. Something needs to trigger the encoding of a string literal with more than 262140 chars.
Any suggestion on which spirv operation to use in such a test?

kuhar · 2025-06-05T14:12:10Z

mlir/include/mlir/Target/SPIRV/SPIRVBinaryUtils.h

+// Max number of words
+constexpr uint32_t kMaxWordCount = 65535;
+
+// Max number of words for literal


Suggested change

// Max number of words

constexpr uint32_t kMaxWordCount = 65535;

// Max number of words for literal

/// Max number of words

constexpr uint32_t kMaxWordCount = 65535;

/// Max number of words for literal

kuhar · 2025-06-05T14:12:52Z

mlir/lib/Target/SPIRV/SPIRVBinaryUtils.cpp

  auto encodingSize = literal.size() / 4 + 1;
+  auto sizeOfDataToCopy = literal.size();


nit: We shouldn't use auto here: https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable

kuhar · 2025-06-05T14:13:24Z

mlir/lib/Target/SPIRV/SPIRVBinaryUtils.cpp

@@ -68,7 +71,18 @@ void spirv::encodeStringLiteralInto(SmallVectorImpl<uint32_t> &binary,
                                    StringRef literal) {
  // We need to encode the literal and the null termination.
  auto encodingSize = literal.size() / 4 + 1;
+  auto sizeOfDataToCopy = literal.size();
+  if (encodingSize >= kMaxLiteralWordCount) {
+    // reserve one word for the null termination


Suggested change

// reserve one word for the null termination

// Reserve one word for the null termination.

See https://llvm.org/docs/CodingStandards.html#commenting

kuhar · 2025-06-05T14:13:33Z

mlir/lib/Target/SPIRV/SPIRVBinaryUtils.cpp

+  if (encodingSize >= kMaxLiteralWordCount) {
+    // reserve one word for the null termination
+    encodingSize = kMaxLiteralWordCount - 1;
+    // do not override the last word (null termination) when copying


Suggested change

// do not override the last word (null termination) when copying

// Do not override the last word (null termination) when copying.

kuhar · 2025-06-05T14:13:52Z

mlir/lib/Target/SPIRV/SPIRVBinaryUtils.cpp

+    // do not override the last word (null termination) when copying
+    sizeOfDataToCopy = (encodingSize - 1) * 4;
+    LLVM_DEBUG(llvm::dbgs() << "Truncating string literal to max size ("
+                            << std::to_string(kMaxLiteralWordCount - 1)


no need to call to_string

kuhar · 2025-06-05T14:15:08Z

mlir/include/mlir/Target/SPIRV/SPIRVBinaryUtils.h

@@ -30,6 +30,12 @@ constexpr uint32_t kMagicNumber = 0x07230203;
 /// The serializer tool ID registered to the Khronos Group
 constexpr uint32_t kGeneratorNumber = 22;

+// Max number of words
+constexpr uint32_t kMaxWordCount = 65535;


Could you either reference the SPIR-V spec or explain why this is the maximum num of words?

IgWod-IMG · 2025-06-05T14:53:48Z

Any suggestion on which spirv operation to use in such a test?

I thought that maybe spirv.EntryPoint would work but no success. I also tried spirv.ExecutionMode. None of my attempts reach your code. So, I’m afraid that I cannot help.

[mlir][spirv] Truncate Literal String size at max number words

54172aa

If not truncated the SPIRV serialization would not fail but instead produce an invalid SPIR-V module. Change-Id: I54c4e54d6ad081861b524d4ae236a1e5080b88c4 Signed-off-by: Davide Grohmann <davide.grohmann@arm.com>

davidegrohmann requested review from antiagainst and kuhar as code owners June 5, 2025 07:59

llvmbot added mlir:spirv mlir labels Jun 5, 2025

IgWod-IMG reviewed Jun 5, 2025

View reviewed changes

mlir/lib/Target/SPIRV/SPIRVBinaryUtils.cpp Show resolved Hide resolved

kuhar reviewed Jun 5, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[mlir][spirv] Truncate Literal String size at max number words #142916

[mlir][spirv] Truncate Literal String size at max number words #142916

davidegrohmann commented Jun 5, 2025

Uh oh!

github-actions bot commented Jun 5, 2025

Uh oh!

llvmbot commented Jun 5, 2025 •

edited

Loading

Uh oh!

IgWod-IMG left a comment

Uh oh!

Uh oh!

davidegrohmann commented Jun 5, 2025

Uh oh!

kuhar Jun 5, 2025

Uh oh!

kuhar Jun 5, 2025

Uh oh!

kuhar Jun 5, 2025

Uh oh!

kuhar Jun 5, 2025

Uh oh!

kuhar Jun 5, 2025

Uh oh!

kuhar Jun 5, 2025

Uh oh!

IgWod-IMG commented Jun 5, 2025

Uh oh!

Uh oh!

		auto encodingSize = literal.size() / 4 + 1;
		auto sizeOfDataToCopy = literal.size();

	// reserve one word for the null termination
	// Reserve one word for the null termination.

	// do not override the last word (null termination) when copying
	// Do not override the last word (null termination) when copying.

[mlir][spirv] Truncate Literal String size at max number words #142916

Are you sure you want to change the base?

[mlir][spirv] Truncate Literal String size at max number words #142916

Conversation

davidegrohmann commented Jun 5, 2025

Uh oh!

github-actions bot commented Jun 5, 2025

Uh oh!

llvmbot commented Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

IgWod-IMG left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

davidegrohmann commented Jun 5, 2025

Uh oh!

kuhar Jun 5, 2025

Choose a reason for hiding this comment

Uh oh!

kuhar Jun 5, 2025

Choose a reason for hiding this comment

Uh oh!

kuhar Jun 5, 2025

Choose a reason for hiding this comment

Uh oh!

kuhar Jun 5, 2025

Choose a reason for hiding this comment

Uh oh!

kuhar Jun 5, 2025

Choose a reason for hiding this comment

Uh oh!

kuhar Jun 5, 2025

Choose a reason for hiding this comment

Uh oh!

IgWod-IMG commented Jun 5, 2025

Uh oh!

Uh oh!

llvmbot commented Jun 5, 2025 •

edited

Loading