[Mlir-commits] [mlir] [mlir][quant] Fix u8 storage type parsed as signless instead of unsigned (PR #185952)

[llvmlistbot at llvm.org]

https://github.com/ggambon created https://github.com/llvm/llvm-project/pull/185952

# Fix: Quant dialect `u8` keyword creates signless `i8` storage instead of unsigned `ui8`

## Summary

The MLIR quant dialect text parser creates a **signless** `i8` `IntegerType` when parsing the `u8` keyword in quantized type syntax (e.g., `!quant.uniform<u8:f32, 0.003>`), instead of the expected **unsigned** `ui8`. This causes silent type corruption during bytecode serialization round-trips and breaks downstream dialects that inspect the `IntegerType` signedness of the storage type.

## Root Cause

In `mlir/lib/Dialect/Quant/IR/TypeParser.cpp`, the `parseStorageType` function has two code paths for parsing the storage type of a quantized type:

1. **`parseOptionalType` path** (handles `i8`, `ui8`, `si8` syntax) — correctly preserves the `IntegerType` signedness from the parsed type:
   ```cpp
   isSigned = !type.isUnsigned();
   storageTypeWidth = type.getWidth();
   ```

2. **`parseKeyword` path** (handles the `u8` shorthand) — creates a **signless** `IntegerType`:
   ```cpp
   isSigned = false;
   type = parser.getBuilder().getIntegerType(storageTypeWidth);  // BUG: signless!
   ```

The `getIntegerType(unsigned width)` single-argument overload creates a signless `IntegerType`. The correct call is `getIntegerType(unsigned width, bool isSigned)`, where `isSigned=false` maps to `IntegerType::Unsigned`:

```cpp
// mlir/lib/IR/Builders.cpp
IntegerType Builder::getIntegerType(unsigned width, bool isSigned) {
  return IntegerType::get(
      context, width, isSigned ? IntegerType::Signed : IntegerType::Unsigned);
}
```

## Fix

One-line change in `parseStorageType`:

```diff
-    type = parser.getBuilder().getIntegerType(storageTypeWidth);
+    type = parser.getBuilder().getIntegerType(storageTypeWidth, isSigned);
```

Since `isSigned` is always `false` in this branch, this creates an `IntegerType::Unsigned` (`ui8`) instead of `IntegerType::Signless` (`i8`).

## Why This Bug Is Silent

The `QuantizedType` stores signedness in **two redundant places**:

1. **`QuantizationFlags::Signed`** — a bitmask flag on the quant type itself, set during parsing based on the `isSigned` local variable.
2. **`IntegerType` signedness** — the signedness semantics of the storage `IntegerType` (`Signless`, `Signed`, or `Unsigned`).

The quant type **printer** uses `QuantizationFlags::Signed` (via `type.isSigned()`) to decide whether to print `u8` or `i8`:

```cpp
static void printStorageType(QuantizedType type, DialectAsmPrinter &out) {
  unsigned storageWidth = type.getStorageTypeIntegralWidth();
  bool isSigned = type.isSigned();  // Uses flags, NOT IntegerType signedness
  if (isSigned) {
    out << "i" << storageWidth;
  } else {
    out << "u" << storageWidth;
  }
}
```

Since the `flags` are set correctly (unsigned), the printer always outputs `u8` — hiding the fact that the underlying `IntegerType` is signless `i8` instead of unsigned `ui8`.

Additionally, `QuantizedType::verifyInvariants` (`mlir/lib/Dialect/Quant/IR/QuantTypes.cpp:45`) does not enforce consistency between `flags` and the `IntegerType` signedness of `storageType`. It derives `isSigned` from `flags` and validates min/max ranges against it, but never checks that `storageType.getSignedness()` agrees. This means a quant type with `flags=unsigned` but `storageType=i8` (signless) passes verification without error, allowing the inconsistency to propagate silently through the IR.

## How This Manifests

### Bytecode serialization round-trip corruption

MLIR bytecode preserves the full `IntegerType` including its signedness. When a module with unsigned quantized types is serialized to bytecode, the storage type is correctly encoded as `ui8`. But when that bytecode is loaded and printed to text, the quant printer outputs `u8`. Re-parsing from text goes through the buggy `u8` keyword path, which creates signless `i8` instead, so re-serializing to bytecode produces a non-identical file.

Any pipeline that saves/loads bytecode checkpoints (e.g., for resuming compilation from a specific pass) silently corrupts unsigned quantized storage types on every text round-trip.

## Historical Context

This bug has been latent since `IntegerType` gained signedness semantics and has never been reported upstream.

| Date | Commit | Author | Event |
|------|--------|--------|-------|
| Apr 2019 | `13bb8f491a1cb` | @stellaraccident | Creates the quant dialect. `QuantizationFlags::Signed` is the only signedness mechanism — `IntegerType` is always signless. |
| Nov 2019 | `e94a8bfca8f9d` | @River707 | Refactors the parser. The `u8` path uses `IntegerType::get(width, context)` — signless, because that's all that existed at the time. |
| Jan 2020 | `35b685270b41` | @antiagainst | `IntegerType` gains signedness semantics (`Signless`, `Signed`, `Unsigned`). The quant parser is not updated. |
| Sep 2021 | `49af2a62758a9` | @jeanPerier | Adds `parseOptionalType` path for explicit `i8`/`ui8`/`si8` syntax. This path correctly reads signedness from the `IntegerType`. The `u8` keyword path is left unchanged — still calling `getIntegerType(storageTypeWidth)` which creates signless. The two code paths now diverge in behavior. |
| Sep 2024 | `852b64862461` | @rafaelubalmw | Full revamp of the quant dialect (#100667). The `parseStorageType` function is not modified. |

## Test Plan

### Bytecode round-trip test (`Bytecode/u8-storage-roundtrip.mlir`)

Uses the explicit `ui8` syntax (which always creates correct unsigned storage via `parseOptionalType`) as the source, emits bytecode, round-trips through text (where the `u8` keyword path re-parses the type), and compares bytecodes with `cmp`. The `--strip-debuginfo` flags remove file path locations that would cause spurious differences.

```mlir
// RUN: mlir-opt %s -allow-unregistered-dialect --strip-debuginfo -emit-bytecode -o %t.bc
// RUN: mlir-opt %t.bc -allow-unregistered-dialect | mlir-opt -allow-unregistered-dialect --strip-debuginfo -emit-bytecode -o %t2.bc
// RUN: cmp %t.bc %t2.bc

module @u8StorageRoundtrip attributes {
  bytecode.test = !quant.uniform<ui8:f32, 9.987200e-01:127>
} {}
```

Without the fix, `cmp` reports a single-byte difference: `0x45` (unsigned `ui8`) vs `0x41` (signless `i8`).

>From 2639be8508e3adf7c3fa14f9e8b12b3307bc8a15 Mon Sep 17 00:00:00 2001
From: German Gambon <ggambon at tesla.com>
Date: Wed, 11 Mar 2026 10:16:19 -0700
Subject: [PATCH] [mlir][quant] Fix u8 storage type parsed as signless instead
 of unsigned

The `parseStorageType` function in the quant dialect has two code paths:
the `parseOptionalType` path (for `i8`/`ui8`/`si8` syntax) correctly
preserves IntegerType signedness, but the `parseKeyword` path (for the
`u8` shorthand) calls `getIntegerType(width)` which creates a signless
`i8` instead of unsigned `ui8`.

This causes silent type corruption: the quant printer uses its own
`QuantizationFlags::Signed` flag (which is set correctly) to print `u8`,
hiding the fact that the underlying IntegerType is signless. The
mismatch surfaces during bytecode serialization round-trips, where the
IntegerType signedness is preserved in bytecode but lost when re-parsed
from text through the `u8` keyword path.

The fix passes the `isSigned` variable (always `false` in this branch)
to `getIntegerType(width, isSigned)`, which creates an unsigned `ui8`.

Co-Authored-By: German Gambon <german.gambon at gmail.com>
---
 mlir/lib/Dialect/Quant/IR/TypeParser.cpp           |  2 +-
 .../Quant/Bytecode/u8-storage-roundtrip.mlir       | 14 ++++++++++++++
 2 files changed, 15 insertions(+), 1 deletion(-)
 create mode 100644 mlir/test/Dialect/Quant/Bytecode/u8-storage-roundtrip.mlir

diff --git a/mlir/lib/Dialect/Quant/IR/TypeParser.cpp b/mlir/lib/Dialect/Quant/IR/TypeParser.cpp
index 1a42b90ac31e2..eef152bf49946 100644
--- a/mlir/lib/Dialect/Quant/IR/TypeParser.cpp
+++ b/mlir/lib/Dialect/Quant/IR/TypeParser.cpp
@@ -48,7 +48,7 @@ static Type parseStorageType(DialectAsmParser &parser, bool &isSigned) {
         return nullptr;
       }
       isSigned = false;
-      type = parser.getBuilder().getIntegerType(storageTypeWidth);
+      type = parser.getBuilder().getIntegerType(storageTypeWidth, isSigned);
     } else {
       parser.emitError(typeLoc, "illegal storage type prefix");
       return nullptr;
diff --git a/mlir/test/Dialect/Quant/Bytecode/u8-storage-roundtrip.mlir b/mlir/test/Dialect/Quant/Bytecode/u8-storage-roundtrip.mlir
new file mode 100644
index 0000000000000..0206788eac725
--- /dev/null
+++ b/mlir/test/Dialect/Quant/Bytecode/u8-storage-roundtrip.mlir
@@ -0,0 +1,14 @@
+// Verify that the u8 keyword in quant types creates unsigned (ui8) storage,
+// not signless (i8). The quant printer always outputs "u8" regardless of the
+// underlying IntegerType signedness, so the only way to detect a mismatch is
+// to compare bytecodes: one from the explicit "ui8" syntax (parsed via
+// parseOptionalType, always unsigned) and one from a text roundtrip through
+// the "u8" keyword path.
+
+// RUN: mlir-opt %s -allow-unregistered-dialect --strip-debuginfo -emit-bytecode -o %t.bc
+// RUN: mlir-opt %t.bc -allow-unregistered-dialect | mlir-opt -allow-unregistered-dialect --strip-debuginfo -emit-bytecode -o %t2.bc
+// RUN: cmp %t.bc %t2.bc
+
+module @u8StorageRoundtrip attributes {
+  bytecode.test = !quant.uniform<ui8:f32, 9.987200e-01:127>
+} {}