[mlir][dxsa] Add BinaryWriter to translate from MLIR to DXSA by asavonic · Pull Request #113 · access-softek/llvm-project

asavonic · 2026-04-27T13:49:00Z

BinaryWriter translates from an MLIR module in DXSA dialect into a DXSA binary. It is a reverse of what BinaryParser does.

Current implementation only supports standard instructions, and needs to be extended to support custom instructions.

Instruction table is moved into a separate file (InstrInfo.def), so it can be shared between Parser/Writer, which build different data structures from it. Parser goes from opcodes to mnemonics, and Writer is reversed.

Tests are extended to run MLIR in roundtrip to verify both the Parser and Writer. We also compare binary output with input to make sure that we do not lose any data during translation.

asl · 2026-04-28T19:37:06Z

@@ -1,4 +1,8 @@
 // RUN: mlir-translate --import-dxsa-bin | FileCheck %s
+// RUN: mlir-translate --export-dxsa-bin %s -o - | mlir-translate --import-dxsa-bin - | FileCheck %s


You can probably introduce --verify-roundtrip, similar to what mlir-op does for normal MLIR

Good idea. I'll see how to add an option like this.

asl · 2026-04-28T19:41:15Z

+    Region &region = source.getRegion();
+    if (!region.hasOneBlock()) {
+      emitError(region.getLoc(), "region should contain only one block");
+      return failure();


emitError produces InFlightDiagnostics that is implicitly convertible to LogicalResult. This could be simplified down to:

return emitError(region.getLoc() , "region should contain only one block");

Ditto in other places.

Great! I missed that.

asl · 2026-04-28T19:42:10Z

+
+    for (auto &op : region.front()) {
+      if (auto inst = dyn_cast<dxsa::Instruction>(op)) {
+        if (failed(emitInstruction(inst))) {


When doing something like this, please ensure that all paths of emitInstruction() produce meaningful diagnostics. Otherwise, you'll end with failed run w/o proper understanding what was wrong.

Sure, that is expected. These patterns only propagate an error up.

asl · 2026-04-28T19:44:15Z

+        return failure();
+      }
+
+      if (auto operand = dyn_cast<dxsa::Operand>(*op)) {


You may probably want to use TypeSwitch here.

Added TypeSwitch here and for indices as well.

asl · 2026-04-28T19:46:01Z

+      if (!index) {
+        emitError(value.getLoc(), "index must be defined");
+        return failure();
+      }
+
+      FailureOr<uint32_t> repr = getIndexRepresentation(index);
+      if (failed(repr)) {


Is it really a "normal" error"? How can we have value out of the thin air?

It is not a normal error. I suppose it can only happen if a value is a block argument, but we do not have control flow for this dialect. Do you suggest to make it an assertion?

This should be enforced by IR then. I mean, if we're having graph regions of SSA dominance.

Replaced these errors by assertions.
All instructions are in a builtin module op, which cannot have block arguments.

asl · 2026-04-28T19:46:25Z

+        return failure();
+      }
+
+      if (auto indexImm = dyn_cast<dxsa::IndexImm>(*index)) {


See above wrt TypeSwitch

asl · 2026-04-28T19:48:10Z

+  // operands do not have indices. They are encoded as an operand
+  // followed by N immediate values for each component.
+  LogicalResult emitOperandImm(dxsa::OperandImm op) {
+    auto attr = dyn_cast<DenseIntElementsAttr>(op.getImm());


do we have verify on dxsa::OperandImm that ensures proper type here? If not, then it should be added and / or type constraint added to the instruction definition.

There is AnyAttrOf<[I32ElementsAttr, I64ElementsAttr]> constraint, but getImm returns mlir::Attribute, so we need to cast it here. Perhaps we can have an assertion instead, but I'm not sure if anything other than DenseIntElementsAttr is possible.

here is how the things are defined:

class SignlessIntElementsAttr<int width> : IntElementsAttrBase< CPred<"::llvm::cast<::mlir::DenseIntElementsAttr>($_self).getType()." "getElementType().isSignlessInteger(" # width # ")">, width # "-bit signless integer elements attribute"> { // Note that this is only constructing scalar elements attribute. let constBuilderCall = "::llvm::cast<::mlir::DenseIntElementsAttr>(" "::mlir::DenseElementsAttr::get(" "::mlir::RankedTensorType::get({}, $_builder.getIntegerType(" # width # ")), " "::llvm::ArrayRef($0)))"; } def I32ElementsAttr : SignlessIntElementsAttr<32>; def I64ElementsAttr : SignlessIntElementsAttr<64>;

So it is always DenseIntElementsAttr and this is enforced by verifier. You should just do direct cast here, no check / error is needed.

asl · 2026-04-28T19:48:56Z

+    // encoded as is, and 64 bit immediates are split into high and
+    // low 32 bit parts.
+    SmallVector<uint32_t, 4> values;
+    for (APInt v : attr) {


you might probably want to grab things by const reference in order not to copy every APInt here.

asl · 2026-04-28T19:50:38Z

+    }
+
+    buffer.push_back(token);
+    for (uint32_t v : values) {


llvm::append_range(buffer, values).

asavonic · 2026-04-29T11:59:47Z

Thanks a lot Anton! Let me know if I can improve anything else.
We have some of these problems in BinaryParser as well. I'll fix them separately.

asl · 2026-04-29T18:36:23Z

+    } else if (elementType.isInteger(64)) {
+      token |= ENCODE_D3D10_SB_OPERAND_TYPE(D3D10_SB_OPERAND_TYPE_IMMEDIATE64);
+    } else {
+      return emitError(op.getLoc(), "invalid immediate operand type");


see above. You can probably also TypeSwitch here with unreachable on default case.

Thank you. Only IntegerType is possible here, so added cast<IntegerType> and an assert to only allow 32- and 64- bit integers.

asl · 2026-04-29T18:38:10Z

+  // Emit an immediate index. Its type is encoded into the operand, so
+  // here we only emit the value as tokens.
+  LogicalResult emitIndexImm(dxsa::IndexImm op) {
+    auto attr = dyn_cast<IntegerAttr>(op.getImm());


See above on cast thing. We can always assume that IR is valid, IR verifier enforces this. So, you'd assert instead of error out.

asl · 2026-04-29T18:38:58Z

+      return emitError(index.getLoc(), "index must be defined");
+    }
+
+    auto operand = dyn_cast<dxsa::Operand>(*def);


You could probably combine the checks into dyn_cast_if_present? Or just cast depending on IR invariants. Same everywhere else.

Done. We can just cast it.

BinaryWriter translates from an MLIR module in DXSA dialect into a DXSA binary. It is a reverse of what BinaryParser does. Current implementation only supports standard instructions, and needs to be extended to support custom instructions. Instruction table is moved into a separate file (InstrInfo.def), so it can be shared between Parser/Writer, which build different data structures from it. Parser goes from opcodes to mnemonics, and Writer is reversed. Tests are extended to run MLIR in roundtrip to verify both the Parser and Writer. We also compare binary output with input to make sure that we do not lose any data during translation.

…eSwitch

… the verifier

asavonic requested a review from tagolog April 27, 2026 13:49

asl reviewed Apr 28, 2026

View reviewed changes

asl reviewed Apr 29, 2026

View reviewed changes

asavonic added 6 commits May 9, 2026 00:02

Return emitError

ef42ce8

Use TypeSwitch

82468a7

Fix for loops

e455a8e

Comment why we only handle Instructions in emitModule

c4948cd

Replace checks for invariants with casts and assertions, add more Typ…

6657041

…eSwitch

asavonic force-pushed the dxsa-mlir-writer branch from 2d21ed3 to 6657041 Compare May 8, 2026 15:20

asavonic added 4 commits May 14, 2026 00:49

Move "invalid number of swizzle values" error to the verifier

461ed9c

Use ModuleOp::getBody

778421d

Move "invalid number of components" error to the verifier

8ef2d15

Move "immediate operand should be either 1- or 4- component" error to…

f7b2fca

… the verifier

		@@ -1,4 +1,8 @@
		// RUN: mlir-translate --import-dxsa-bin \| FileCheck %s
		// RUN: mlir-translate --export-dxsa-bin %s -o - \| mlir-translate --import-dxsa-bin - \| FileCheck %s

Conversation

asavonic commented Apr 27, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asl Apr 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asavonic commented Apr 29, 2026

Uh oh!

asl Apr 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asl Apr 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

asl Apr 28, 2026 •

edited

Loading

asl Apr 29, 2026 •

edited

Loading

asl Apr 29, 2026 •

edited

Loading