[Sim][MooreToCore] Added width, alignment and padding support for `moore.fmt.int` #9390

KavyaChopra04 · 2025-12-26T21:02:19Z

The standard wasn't super clear about the semantics, so here's what I figured out after twiddling with some test cases using Icarus:

For Octal, Hexadecimal, and Binary numbers, they are padded with 0s upto the minimum width that a number of that datatype would fit into (eg i22 would fit into (22 + 2)/3 = 8 octal bytes, so 106 in octal would be printed as 00000152), and then any additional bytes are padded with spaces (so $write("%10o",val) would be 00000152). Even contraction would have the whole 0-padded value be printed ($write("%3o",val) would still be 00000152). A similar logic follows for hexadecimal and binary
For decimal numbers, the default padding is spaces themselves, but they can contract too. $write("%4d",val) (where val is an i22 with value 106) would cause 106 to be printed (an extra space to fill up the padding of 4). Expansion is pretty straightforward, and similar to Octals. Signed numbers can also require an extra character to be printed sometimes.

This PR implements the above, and adds some test cases to substantiate them. Happy to add more tests if needed as well

lib/Dialect/Sim/SimOps.cpp

test/Dialect/Sim/format-strings.mlir

test/Conversion/ImportVerilog/builtins.sv

fabianschuiki

Thanks a lot for reworking this and bringing it up to par with the SV spec 🥳👍. Just a few minor nits and potential to reduce code duplication a bit.

include/circt/Dialect/Moore/MooreOps.td

fabianschuiki · 2026-01-02T18:07:26Z

lib/Conversion/ImportVerilog/FormatStrings.cpp

+    mlir::IntegerAttr widthAttr = nullptr;
+    if (options.width) {
+      widthAttr = builder.getI32IntegerAttr(*options.width);
+    }


Nit:

Suggested change

mlir::IntegerAttr widthAttr = nullptr;

if (options.width) {

widthAttr = builder.getI32IntegerAttr(*options.width);

}

IntegerAttr widthAttr = nullptr;

if (options.width)

widthAttr = builder.getI32IntegerAttr(*options.width);

fabianschuiki · 2026-01-02T18:08:11Z

lib/Conversion/MooreToCore/MooreToCore.cpp

-    // TODO: These should honor the width, alignment, and padding.
+
+    char padChar = adaptor.getPadding() == IntPadding::Space ? 32 : 48;
+    mlir::IntegerAttr padCharAttr = rewriter.getI8IntegerAttr(padChar);


Suggested change

mlir::IntegerAttr padCharAttr = rewriter.getI8IntegerAttr(padChar);

IntegerAttr padCharAttr = rewriter.getI8IntegerAttr(padChar);

fabianschuiki · 2026-01-02T18:08:21Z

lib/Conversion/MooreToCore/MooreToCore.cpp

+    auto widthAttr = adaptor.getSpecifierWidthAttr();
+
+    bool isLeftAligned = adaptor.getAlignment() == IntAlign::Left;
+    mlir::BoolAttr isLeftAlignedAttr = rewriter.getBoolAttr(isLeftAligned);


Suggested change

mlir::BoolAttr isLeftAlignedAttr = rewriter.getBoolAttr(isLeftAligned);

BoolAttr isLeftAlignedAttr = rewriter.getBoolAttr(isLeftAligned);

fabianschuiki · 2026-01-02T18:13:04Z

lib/Dialect/Sim/SimOps.cpp

    SmallVector<char, 8> strBuf;
    intAttr.getValue().toString(strBuf, 16U, /*Signed*/ false,
                                /*formatAsCLiteral*/ false,
-                                /*UpperCase*/ false);
+                                /*UpperCase*/ adaptor.getIsHexUppercase());

    unsigned width = intAttr.getType().getIntOrFloatBitWidth();
    unsigned padWidth = width / 4;
    if (width % 4 != 0)
      padWidth++;
+
+    unsigned numSpaces;
+    if (adaptor.getSpecifierWidth().has_value()) {
+      numSpaces = std::max(
+          0U, adaptor.getSpecifierWidth().value() -
+                  std::max(padWidth, static_cast<unsigned>(strBuf.size())));
+    } else {
+      numSpaces = 0;
+    }
+
+    SmallVector<char, 1> spacePadding(numSpaces, ' ');
    padWidth = padWidth > strBuf.size() ? padWidth - strBuf.size() : 0;

-    SmallVector<char, 8> padding(padWidth, '0');
-    return StringAttr::get(getContext(), Twine(padding) + Twine(strBuf));
+    SmallVector<char, 8> padding(padWidth, adaptor.getPaddingChar());
+    if (adaptor.getIsLeftAligned()) {
+      return StringAttr::get(getContext(), Twine(padding) + Twine(strBuf) +
+                                               Twine(spacePadding));
+    }
+    return StringAttr::get(getContext(), Twine(spacePadding) + Twine(padding) +
+                                             Twine(strBuf));
  }


Any chance you could factor this code out into a helper function that unifies formatting for binary, octal, hexadecimal, and decimal? I think this only differs by the base passed to the int's toString().

fabianschuiki · 2026-01-02T18:16:35Z

test/Conversion/MooreToCore/basic.mlir

-  // CHECK: sim.fmt.hex %arg0 : i42
-  moore.fmt.int hex_upper %arg0, width 42, align right, pad space : i42
+  // CHECK: sim.fmt.dec %arg0 {specifierWidth = 42 : i32} : i42
+  moore.fmt.int decimal %arg0, align right, pad space, width 42 : i42


Can you add a test for signed but no width after this?

// CHECK: sim.fmt.dec %arg0 {isSigned = true} : i42 moore.fmt.int decimal %arg0, align right, pad space, signed : i42

fabianschuiki · 2026-01-02T18:17:38Z

test/Conversion/MooreToCore/basic.mlir

-}
+}


Nit: Stray whitespace change. Keep a newline at the end of the file.

fabianschuiki · 2026-01-02T18:18:34Z

test/Dialect/Moore/basic.mlir

+  // CHECK: moore.fmt.int decimal %arg1, align left, pad zero, width 42, signed : i42
+  moore.fmt.int decimal %arg1, align left, pad zero, width 42, signed : i42


Can you check that the parser also eats the following? I suspect it might not 😬:

// CHECK: moore.fmt.int decimal %arg1, align left, pad zero, signed : i42 moore.fmt.int decimal %arg1, align left, pad zero, signed : i42

fabianschuiki · 2026-01-02T18:20:03Z

test/Dialect/Sim/format-strings.mlir

+  %wrd = sim.fmt.dec %1 : i16
+  %wld = sim.fmt.dec %1 {isLeftAligned = true}: i16
+  %wrds = sim.fmt.dec %1 {isSigned} : i16
+  %wlds = sim.fmt.dec %1 {isLeftAligned = true, isSigned}: i16
+  %wrdp = sim.fmt.dec %1 {specifierWidth = 10 : i32}: i16
+  %wldp = sim.fmt.dec %1 {isLeftAligned = true, specifierWidth = 10 : i32}: i16
+  %wrdps = sim.fmt.dec %1 {specifierWidth = 10 : i32, isSigned}: i16
+  %wldps = sim.fmt.dec %1 {isLeftAligned = true, specifierWidth = 10 : i32, isSigned}: i16
+  %dec = sim.fmt.concat (%wrd, %comma, %wld, %comma, %wrds, %comma, %wlds, %comma, %wrdp, %comma, %wldp, %comma, %wrdps, %comma, %wldps)


Thanks for testing all this! 🖐️

fabianschuiki · 2026-01-02T18:21:25Z

test/Dialect/Sim/proceduralize-sim.mlir

-// CHECK-DAG:      %[[FD:.*]] = sim.fmt.dec %[[ARG]] : i32
-// CHECK-DAG:      %[[FH:.*]] = sim.fmt.hex %[[ARG]] : i32
-// CHECK-DAG:      %[[FO:.*]] = sim.fmt.oct %[[ARG]] : i32
+// CHECK-DAG:      %[[FB:.*]] = sim.fmt.bin %[[ARG]] {specifierWidth = 32 : i32} : i32


Nit: This makes me wonder if we should just call the optional width width instead of specifierWidth. No need to change anything in the PR 😁 Just a thought for a later refactoring.

My rationale to go with specifierWidth was to disambiguate the datatype width (which is used when you don't add anything to the specifier, eg in %d and is inferred in SimOps.cpp) vs the specifier width (which is used when you explicitly specify the width you want, eg %10d, which we inherit from the input file itself). Should we still rename it to width?

…gnment

fabianschuiki

Thanks for all the tweaks and factoring out the code 🥳 🙌. I've rebased this onto main and fixed a few breakages with the time formatting that landed on main in the meantime. Going to merge this as soon as CI is happy!

preechapon250 mentioned this pull request Dec 27, 2025

[FIRRTL][HW] Improved Optimizations for Observability #9384

Open

3 tasks

tobiasgrosser reviewed Dec 30, 2025

View reviewed changes

lib/Dialect/Sim/SimOps.cpp Outdated Show resolved Hide resolved

tobiasgrosser reviewed Dec 30, 2025

View reviewed changes

test/Dialect/Sim/format-strings.mlir Outdated Show resolved Hide resolved

tobiasgrosser reviewed Dec 30, 2025

View reviewed changes

test/Conversion/ImportVerilog/builtins.sv Outdated Show resolved Hide resolved

KavyaChopra04 mentioned this pull request Dec 30, 2025

[MooreToCore][Sim] Added support for moore.fmt.real #9397

Merged

fabianschuiki approved these changes Jan 2, 2026

View reviewed changes

fabianschuiki added Moore Simulator Involving the Sim dialect or other simulation concerns labels Jan 2, 2026

Kavya Chopra and others added 15 commits January 5, 2026 09:22

added padding to format int ops

5c85e56

added correct specifier width logic

ef51bc9

fixed tests and assembly formatting

9fe05d5

added in unsigned bitwidth logic + fixed format strings tests

8434a26

modified source code to use opt attrs and account for padding and ali…

8471d11

…gnment

FINALLY got the tests to pass omg

486d79b

added hex upper/lower to tests

107a4f4

added more tests, corrected non decimal padding logic

7230716

removed spurious print

34c3247

nicer formatting in moore2core

3848cb7

some more formatting fixes

43a1c30

fixed sim assembly formatting + removed mlir namespace stuff

e565e25

fixed moore assembly

8e85f9b

refactored dec/oct/hex code

365fd00

Fixes after rebase onto main

4314042

fabianschuiki force-pushed the fmt-int-full-support branch from b8dfbad to 4314042 Compare January 5, 2026 17:26

fabianschuiki approved these changes Jan 5, 2026

View reviewed changes

fabianschuiki merged commit 197dc38 into llvm:main Jan 5, 2026
7 checks passed

	mlir::IntegerAttr padCharAttr = rewriter.getI8IntegerAttr(padChar);
	IntegerAttr padCharAttr = rewriter.getI8IntegerAttr(padChar);

	mlir::BoolAttr isLeftAlignedAttr = rewriter.getBoolAttr(isLeftAligned);
	BoolAttr isLeftAlignedAttr = rewriter.getBoolAttr(isLeftAligned);

		// CHECK: moore.fmt.int decimal %arg1, align left, pad zero, width 42, signed : i42
		moore.fmt.int decimal %arg1, align left, pad zero, width 42, signed : i42

[Sim][MooreToCore] Added width, alignment and padding support for moore.fmt.int #9390

[Sim][MooreToCore] Added width, alignment and padding support for moore.fmt.int #9390

Uh oh!

Conversation

KavyaChopra04 commented Dec 26, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fabianschuiki left a comment

Choose a reason for hiding this comment

Uh oh!

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!

fabianschuiki left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

[Sim][MooreToCore] Added width, alignment and padding support for `moore.fmt.int` #9390

[Sim][MooreToCore] Added width, alignment and padding support for `moore.fmt.int` #9390