dialects: Add print_int, print_char ops. #1311

JosseVanDelm · 2023-07-20T15:00:34Z

Hi, this is an initial PR for adding more self-contained printing capabilities to the printf dialect.
I'm still unsure about a lot of stuff, and it is not finished yet, some initial help is appreciated!

This PR adds:

Two new ops:

A "printf.print_char" op. An operation that prints a single character, that is taken in as a 32-bit integer.
A "printf.print_int" op. An operation that prints a 32 bit integer.
One pass lower-printchar-to-putchar (naming suggestions welcome!) that implements:
A rewrite from print_int to a func.func inline_itoa calls. The implementation of inline_itoa is inserted in the pass. Inline_itoa was written in MLIR assembly directly.
A rewrite that lowers print_chars to C style put_chars.

Rationale

Some platforms don't ship with printf implementations or are very bad at printing. This implementation allows for printing integers:

without relying much on external libraries (besides put_char)
at (maybe?) increased performance, since now MLIR-passes can actually perform constant folding and the likes.
I think we can (if necessary) even write a pass that composes the current printf.format() into print_chars and print_ints if necessary.

Why a print_char op?

Because I felt like using putchar directly was a bit ugly (it returns what it puts, which you typically don't really need, I believe, and because some other platforms might implement these calls in a different way than the standard C approach.

Why a print_int op?

Some platforms implement this directly. It is for example available as a RISC-V syscall.

Still missing/Questions/Concerns

It feels like I'm not implementing signless semantics properly (should I make a signed print and unsigned print instead?). The implementation currently tests if the value is negative, and then prints a minus sign if necessary.
I feel like the added itoa routine is not very easy to read and/or review. Any ideas on making this easier?
Should I keep this limited to 32-bit integers?
Some ops in the conversion seem to be still missing in xdsl (math.log10?), I'll work on these next.

codecov · 2023-07-20T15:04:43Z

Codecov Report

Patch coverage: 95.33% and project coverage change: +0.03% 🎉

Comparison is base (23153ed) 90.03% compared to head (0ec51de) 90.06%.
Report is 33 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1311      +/-   ##
==========================================
+ Coverage   90.03%   90.06%   +0.03%     
==========================================
  Files         211      215       +4     
  Lines       26266    26815     +549     
  Branches     3902     3986      +84     
==========================================
+ Hits        23649    24152     +503     
- Misses       2004     2025      +21     
- Partials      613      638      +25

Files Changed	Coverage Δ
xdsl/tools/command_line_tool.py	`100.00% <ø> (ø)`
xdsl/transforms/printf_to_putchar.py	`94.23% <94.23%> (ø)`
xdsl/dialects/printf.py	`92.64% <95.23%> (+0.98%)`	⬆️
tests/dialects/test_arith.py	`100.00% <100.00%> (ø)`
tests/dialects/test_printf.py	`100.00% <100.00%> (ø)`
xdsl/dialects/arith.py	`91.40% <100.00%> (+0.25%)`	⬆️

... and 16 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

tests/filecheck/dialects/print/print_basics.mlir

superlopuh · 2023-07-22T11:27:42Z

xdsl/dialects/printf.py

-    [
-        PrintFormatOp,
-    ],
+    [PrintFormatOp, PrintCharOp, PrintIntOp],


Suggested change

[PrintFormatOp, PrintCharOp, PrintIntOp],

[PrintFormatOp, PrintCharOp, PrintIntOp,],

This would be a useful change to minimise spurious git diffs in the future

Sure, but what do you mean with

to minimise spurious git diffs in the future

?

compor

Thanks for providing such detailed info for this PR, it made the review easier.

I mainly focused on itoa.

As for signless integers, my understanding is that treating them as signed is compatible with their semantics and it's up to the corresponding operation to interpret them as required.
Keeping this working just for 32-bit int sounds good to me for now.

compor · 2023-07-22T13:51:09Z

tests/filecheck/dialects/print/printchar_to_putchar.mlir

@@ -0,0 +1,15 @@
+// RUN: xdsl-opt -p lower-printchar-to-putchar %s | xdsl-opt | filecheck %s
+
+builtin.module {


nit: you can omit the builtin.module now and save some indentation space

xdsl/transforms/printf_to_putchar.py

AntonLydike

LGTM with minor corrections!

xdsl/dialects/printf.py

xdsl/transforms/printf_to_putchar.py

xdsl/dialects/printf.py

xdsl/transforms/printf_to_putchar.py

adutilleul

Nice!

xdsl/dialects/printf.py

xdsl/transforms/printf_to_putchar.py

kingiler · 2023-08-03T16:37:46Z

I am happy that printf works and I'm thinking of a way that do not need to use pow.
The following python code looks good despite two while loops:

def reverse(n: int) -> int: # n is absolute value
    m = 0
    while n:
        m = 10 * m + n % 10
        n //= 10
    return m


def print_integer(n: int) -> None:
    m = reverse(n)
    while m:
        print(m % 10, end="") # print single digit as char
        m //= 10


print_integer(123456)

Just some ideas.

JosseVanDelm · 2023-08-03T19:21:34Z

Thanks for the idea @kingiler, i'm not entirely sure if what you are proposing is strictly necessary though, since the mlir-opt --convert-math-to-funcs seem to already implement this behaviour for us, as it inserts a function to perform powers on integers.

…implementation

xdsl/dialects/printf.py

xdsl/transforms/printf_to_putchar.py

superlopuh · 2023-08-08T09:37:41Z

xdsl/transforms/printf_to_putchar.py

+        )
+        print_char_walker.rewrite_module(op)
+        # Add external putchar reference
+        # TODO Don't insert this if the operation is not there!


Would be good to add this functionality as part of this PR. it should be pretty easy with the symbol table functionality, or just a manual scan of the module op body.

Could you give a reference/example to the symbol table functionality? I don't know it. Thanks!

It might not work here, but in theory can be something like in the get_op_for_symbol function in interpreter.py

def get_op_for_symbol(self, symbol: str) -> Operation: if symbol in self.symbol_table: return self.symbol_table[symbol] else: raise InterpretationError(f'Could not find symbol "{symbol}"') @property def symbol_table(self) -> dict[str, Operation]: if self._symbol_table is None: self._symbol_table = {} for op in self.module.walk(): if (symbol_interface := op.get_trait(SymbolOpInterface)) is not None: symbol = symbol_interface.get_sym_attr_name(op) if symbol: self._symbol_table[symbol.data] = op return self._symbol_table

Hmm actually maybe just this will work: SymbolTable.lookup_symbol(op, "foo")

If it doesn't work I'd be interested :)

Can you doublecheck now

JosseVanDelm · 2023-08-09T17:44:06Z

As per @AntonLydike's suggestion I've removed the inline_itoa and refactored it to mlir_itoa, which seems to be what mlir-opt does when you lower a math.ipowi with convert-math-to-funcs

tests/dialects/test_printf.py

xdsl/dialects/printf.py

xdsl/transforms/printf_to_putchar.py

tests/dialects/test_printf.py

xdsl/transforms/printf_to_putchar.py

kingiler · 2023-08-11T15:44:43Z

xdsl/dialects/printf.py

    var_operand_def,
 )
 from xdsl.parser import Parser
 from xdsl.printer import Printer

+i8 = builtin.IntegerType(8)


I am not sure but i8 seems useful to be put in xdsl/dialect/builtin.py?

Also not sure, I don't think it matters that much though, it would save about one line in code

superlopuh

Looking good, some minor changes would be nice to make or discuss

xdsl/transforms/printf_to_putchar.py

superlopuh · 2023-08-12T16:00:50Z

xdsl/dialects/printf.py

-    [
-        PrintFormatOp,
-    ],
+    [PrintFormatOp, PrintCharOp, PrintIntOp],


superlopuh

Looking forward to using this :)

JosseVanDelm · 2023-08-14T12:51:38Z

Thank you all for the reviews! I learned a lot while working on this!

JosseVanDelm added dialects Changes on the dialects convolve labels Jul 20, 2023

JosseVanDelm requested review from superlopuh and AntonLydike July 20, 2023 15:00

JosseVanDelm self-assigned this Jul 20, 2023

JosseVanDelm marked this pull request as draft July 20, 2023 15:00

JosseVanDelm requested review from compor and adutilleul July 21, 2023 15:08

superlopuh reviewed Jul 22, 2023

View reviewed changes

tests/filecheck/dialects/print/print_basics.mlir Outdated Show resolved Hide resolved

superlopuh reviewed Jul 22, 2023

View reviewed changes

compor reviewed Jul 22, 2023

View reviewed changes

AntonLydike approved these changes Jul 26, 2023

View reviewed changes

xdsl/dialects/printf.py Outdated Show resolved Hide resolved

xdsl/transforms/printf_to_putchar.py Outdated Show resolved Hide resolved

xdsl/dialects/printf.py Show resolved Hide resolved

xdsl/transforms/printf_to_putchar.py Outdated Show resolved Hide resolved

adutilleul approved these changes Jul 31, 2023

View reviewed changes

xdsl/dialects/printf.py Outdated Show resolved Hide resolved

xdsl/transforms/printf_to_putchar.py Outdated Show resolved Hide resolved

xdsl/transforms/printf_to_putchar.py Outdated Show resolved Hide resolved

JosseVanDelm force-pushed the Josse/add-print-int branch from 36e526c to d00e1f0 Compare August 3, 2023 11:56

kingiler reviewed Aug 3, 2023

View reviewed changes

xdsl/transforms/printf_to_putchar.py Outdated Show resolved Hide resolved

JosseVanDelm added 12 commits August 7, 2023 15:35

dialects: Add print_char operation

19170eb

dialects: Add printchar to putchar lowering

19d8ad3

dialects: Add print_int op

79eacec

dialects: Add initial version of itoa func

1f2b0d4

dialects: Fix algorithm

1f4e5b7

Restructure format of itoa

95eb1d5

Extend test for print_char and print_int

2f1ef88

Working while loop

8606a35

dialects: Favor remainder integer division instead of floating point …

0048909

…implementation

printchar: Add from_constant_char constructor

52b0fed

Factor out print_minus_if_negative

34716ab

printchar: Factor out print_digits

e901295

JosseVanDelm requested a review from adutilleul August 8, 2023 09:25

superlopuh changed the title ~~[WIP] dialects: Add print_int, print_char ops.~~ dialects: Add print_int, print_char ops. Aug 8, 2023

superlopuh requested changes Aug 8, 2023

View reviewed changes

adutilleul approved these changes Aug 8, 2023

View reviewed changes

JosseVanDelm added 3 commits August 9, 2023 17:15

dialects: Add constructors and valueerrors

6facd94

dialects: Add extra checks to and rename to printf_to_putchar

baeb22a

dialects: refactor inline_itoa to mlir_itoa

029bf08

dialects: fix lit test and change test name

d3ce4d6

JosseVanDelm requested a review from superlopuh August 9, 2023 18:01

superlopuh reviewed Aug 10, 2023

View reviewed changes

tests/dialects/test_printf.py Show resolved Hide resolved

superlopuh reviewed Aug 10, 2023

View reviewed changes

tests/dialects/test_printf.py Outdated Show resolved Hide resolved

superlopuh reviewed Aug 10, 2023

View reviewed changes

xdsl/dialects/printf.py Outdated Show resolved Hide resolved

superlopuh reviewed Aug 10, 2023

View reviewed changes

xdsl/transforms/printf_to_putchar.py Outdated Show resolved Hide resolved

superlopuh reviewed Aug 10, 2023

View reviewed changes

xdsl/transforms/printf_to_putchar.py Outdated Show resolved Hide resolved

compor approved these changes Aug 11, 2023

View reviewed changes

Address Sasha's comments

f8dc0c3

JosseVanDelm requested a review from superlopuh August 11, 2023 15:30

kingiler reviewed Aug 11, 2023

View reviewed changes

tests/dialects/test_printf.py Outdated Show resolved Hide resolved

kingiler reviewed Aug 11, 2023

View reviewed changes

xdsl/transforms/printf_to_putchar.py Outdated Show resolved Hide resolved

kingiler reviewed Aug 11, 2023

View reviewed changes

xdsl/transforms/printf_to_putchar.py Outdated Show resolved Hide resolved

kingiler reviewed Aug 11, 2023

View reviewed changes

superlopuh requested changes Aug 12, 2023

View reviewed changes

Address comments

0ec51de

JosseVanDelm requested review from superlopuh and kingiler August 14, 2023 08:44

superlopuh approved these changes Aug 14, 2023

View reviewed changes

JosseVanDelm merged commit 32a51a7 into main Aug 14, 2023
10 checks passed

JosseVanDelm deleted the Josse/add-print-int branch August 14, 2023 12:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dialects: Add print_int, print_char ops. #1311

dialects: Add print_int, print_char ops. #1311

JosseVanDelm commented Jul 20, 2023

codecov bot commented Jul 20, 2023 •

edited

superlopuh Jul 22, 2023

superlopuh Aug 10, 2023

superlopuh Aug 12, 2023

JosseVanDelm Aug 14, 2023

compor left a comment •

edited

compor Jul 22, 2023

AntonLydike left a comment

adutilleul left a comment

kingiler commented Aug 3, 2023

JosseVanDelm commented Aug 3, 2023

superlopuh Aug 8, 2023

JosseVanDelm Aug 8, 2023

superlopuh Aug 8, 2023 •

edited

superlopuh Aug 8, 2023

superlopuh Aug 8, 2023

JosseVanDelm Aug 9, 2023

JosseVanDelm commented Aug 9, 2023

kingiler Aug 11, 2023

JosseVanDelm Aug 11, 2023 •

edited

superlopuh left a comment

superlopuh Aug 12, 2023

superlopuh left a comment

JosseVanDelm commented Aug 14, 2023

	[PrintFormatOp, PrintCharOp, PrintIntOp],
	[PrintFormatOp, PrintCharOp, PrintIntOp,],

		@@ -0,0 +1,15 @@
		// RUN: xdsl-opt -p lower-printchar-to-putchar %s \| xdsl-opt \| filecheck %s

		builtin.module {

dialects: Add print_int, print_char ops. #1311

dialects: Add print_int, print_char ops. #1311

Conversation

JosseVanDelm commented Jul 20, 2023

This PR adds:

Rationale

Still missing/Questions/Concerns

codecov bot commented Jul 20, 2023 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compor left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AntonLydike left a comment

Choose a reason for hiding this comment

adutilleul left a comment

Choose a reason for hiding this comment

kingiler commented Aug 3, 2023

JosseVanDelm commented Aug 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

superlopuh Aug 8, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JosseVanDelm commented Aug 9, 2023

Choose a reason for hiding this comment

JosseVanDelm Aug 11, 2023 • edited

Choose a reason for hiding this comment

superlopuh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

superlopuh left a comment

Choose a reason for hiding this comment

JosseVanDelm commented Aug 14, 2023

codecov bot commented Jul 20, 2023 •

edited

compor left a comment •

edited

superlopuh Aug 8, 2023 •

edited

JosseVanDelm Aug 11, 2023 •

edited