transforms: (print) Add a lowering to printf #1142

AntonLydike · 2023-06-15T08:18:33Z

This is a working, minimal lowering to printf!

I have some hacky stuff happening in the ModulePass, let me know if this is undesireable!

AntonLydike · 2023-06-15T08:19:03Z

xdsl/dialects/llvm.py

-        if isinstance(global_name, str):
-            global_name = StringAttr(global_name)
-        if isinstance(global_name, StringAttr):
+        if isinstance(global_name, (StringAttr, str)):
            global_name = SymbolRefAttr(global_name)


This is a trivial simplification, I hope it's okay to have in this PR?

I can not comment, because I don't understand?

codecov · 2023-06-15T08:23:38Z

Codecov Report

Patch coverage: 85.71% and project coverage change: +0.04 🎉

Comparison is base (85b16da) 87.23% compared to head (b6c500a) 87.27%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1142      +/-   ##
==========================================
+ Coverage   87.23%   87.27%   +0.04%     
==========================================
  Files         143      145       +2     
  Lines       21248    21336      +88     
  Branches     3206     3215       +9     
==========================================
+ Hits        18536    18622      +86     
+ Misses       2211     2209       -2     
- Partials      501      505       +4

Impacted Files	Coverage Δ
xdsl/dialects/llvm.py	`89.45% <0.00%> (+2.14%)`	⬆️
xdsl/dialects/print.py	`80.00% <ø> (ø)`
xdsl/transforms/print_to_println.py	`82.60% <82.60%> (ø)`
tests/dialects/test_print.py	`100.00% <100.00%> (ø)`
xdsl/xdsl_opt_main.py	`96.61% <100.00%> (+0.01%)`	⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

xdsl/transforms/print_to_println.py

superlopuh · 2023-06-15T08:38:32Z

tests/filecheck/dialects/print/print_to_printf.mlir

@@ -0,0 +1,20 @@
+// RUN: xdsl-opt %s -p print-to-printf | mlir-opt --test-lower-to-llvm | mlir-translate --mlir-to-llvmir | filecheck %s
+// this tests straight to llvmir, because I feel that that's a reasonable thing to do.


It is in addition to one that doesn't send straight to llvmir :) Also please rephrase the comment to remove the "I", it's a team project

Yep! fixed (and added another test)

xdsl/transforms/print_to_println.py

superlopuh · 2023-06-15T08:40:46Z

xdsl/transforms/print_to_println.py

+                args.append(new_val.result)
+                format_str += "%li"
+            elif part.typ == builtin.f32:
+                # f32 must be promoted to f64 before printing


I wonder whether this should be done as a separate rewrite, seems kind of dodgy to me. We could borrow python's behaviour of there being some default print format and a trait for conversion to printf-supported types.

hmmm, interesting point. I think this should still happen here though, because the type printing code shouldn't be bothered with the intricacies of the printf function?

Also, we plan on supporting both printf based lowering and a putc based one, so we need to be careful how we generalize here...

@superlopuh I think what you are saying is that you would like to lower to a printfOp first and then from printfOp to putchar?
Because then it makes more sense to me to change the current println op?
Could you be more precise with what you mean with "default print format?"

I mean in general it would be good to know what the semantics are of what is printable and how. I'd rather just fail to print certain types than have implicit conversions for the time being, hopefully clients can work around in the near future.

PrintableType interface is on the horizon, I want to be able to print memrefs, etc!

georgebisbas · 2023-06-15T09:22:16Z

tests/dialects/test_print.py

+        ("test", "test"),
+        ("@hello  world", "hello_world"),
+        ("something.with.dots", "something_with_dots"),
+        ("this is 💩", "this_is"),


I'm thinking maybe we should be using safe escaping like this: https://stackoverflow.com/questions/1695183/how-can-i-percent-encode-url-parameters-in-python so things like emoji are still kept around. I have a feeling this might bite us later if we're doing printing of fairly standard things like "(" and ")"

Emojis will be kept around in the actual string, this builds the global symbol name (and only part of it)

Can you add "(" and ")" as test cases to see what happens? Preferably end-to-end, like what would this do:

print("(") print("a") print(")")

Would it be the hash that resolves the difference between the two?

Given:

print.println "(" print.println ")"

We get:

module { func.func @main() { %0 = llvm.mlir.addressof @_900229d109e2354708da1b4fe903c1ef0e741ab8 : !llvm.ptr llvm.call @printf(%0) : (!llvm.ptr) -> () %1 = llvm.mlir.addressof @_a35077a93b180088463ecc1cdf83178d99a53a66 : !llvm.ptr llvm.call @printf(%1) : (!llvm.ptr) -> () return } llvm.func @printf(!llvm.ptr, ...) // this is the "(" string llvm.mlir.global internal constant @_900229d109e2354708da1b4fe903c1ef0e741ab8(dense<[40, 10, 0]> : tensor<3xi8>) {addr_space = 0 : i32} : !llvm.array<3 x i8> // this is ")" llvm.mlir.global internal constant @_a35077a93b180088463ecc1cdf83178d99a53a66(dense<[41, 10, 0]> : tensor<3xi8>) {addr_space = 0 : i32} : !llvm.array<3 x i8> }

As you can see, the symbol names are no longer useful in identifying the string, but that's okay for me. They are still unique though! And we have relatively strong guarantees for that for as long as the string is not crafted maliciously. And in the worst case scenario is that some printing breaks. This is an acceptable tradeoff imo.

The prefix is just added so that one can (sometimes) tell what string is printed, and I couldn't think of a better way that guaranteed no symbol name clashes.

Just a sidecomment (that does not even help here, AFAIU):

A classic, hacky trick in c++ to get unique numbers is to malloc a single byte and use the pointer to it as the unique number. Maybe you can do something similar here?

I added a pytest test for this, I feel it's useful to test this behavior!

Why not add them here

If I added them here, the test would have to check the exact hashes, and would therefore test an implementation detail.

Instead, I want to test the contract. So that key(x) == key(x) and key(x) != key(y), even for non-legal characters.

I can do that in pytest much easier then I can in filecheck

JosseVanDelm

Mostly a few nits on spelling (your craving for beer is somewhat worrying 🙃 ).
Looks good to me in general!

xdsl/xdsl_opt_main.py

xdsl/transforms/print_to_println.py

JosseVanDelm · 2023-06-15T10:22:10Z

xdsl/transforms/print_to_println.py

+                args.append(new_val.result)
+                format_str += "%li"
+            elif part.typ == builtin.f32:
+                # f32 must be promoted to f64 before printing


@superlopuh I think what you are saying is that you would like to lower to a printfOp first and then from printfOp to putchar?
Because then it makes more sense to me to change the current println op?
Could you be more precise with what you mean with "default print format?"

JosseVanDelm · 2023-06-15T10:23:27Z

xdsl/dialects/llvm.py

-        if isinstance(global_name, str):
-            global_name = StringAttr(global_name)
-        if isinstance(global_name, StringAttr):
+        if isinstance(global_name, (StringAttr, str)):
            global_name = SymbolRefAttr(global_name)


I can not comment, because I don't understand?

tests/dialects/test_print.py

superlopuh · 2023-06-16T07:40:36Z

tests/dialects/test_print.py

+        ("test", "test"),
+        ("@hello  world", "hello_world"),
+        ("something.with.dots", "something_with_dots"),
+        ("this is 💩", "this_is"),


Can you add "(" and ")" as test cases to see what happens? Preferably end-to-end, like what would this do:

print("(") print("a") print(")")

Would it be the hash that resolves the difference between the two?

superlopuh · 2023-06-16T07:43:56Z

xdsl/transforms/print_to_println.py

+                args.append(new_val.result)
+                format_str += "%li"
+            elif part.typ == builtin.f32:
+                # f32 must be promoted to f64 before printing


I mean in general it would be good to know what the semantics are of what is printable and how. I'd rather just fail to print certain types than have implicit conversions for the time being, hopefully clients can work around in the near future.

superlopuh

LGTM modulo other comments, if you don't think it's critical I think we can merge, just would like to avoid silent gotchas in the future

webmiche

LGTM, nice to see this evolve 👍

Did you already run lowered code and does it work?

AntonLydike · 2023-06-16T08:49:42Z

LGTM, nice to see this evolve +1

Thanks!

Did you already run lowered code and does it work?

Yep! Already debugged devito lowerings with it!

JosseVanDelm

Thank you for working on this @AntonLydike!

This adds the `print-to-printf` pass to lower println ops to printf calls

Now that Toy lowers to print_format, we can compile it to LLVM, leveraging the printf flow added in #1142 .

AntonLydike added 4 commits June 15, 2023 08:27

print: initial println op added

9003dc9

print: start of the println lowering

416f3a0

print: println lowering improvements

28f4585

print: printf lowering working now

f4fbc62

AntonLydike added the transformations Changes or adds a transformatio label Jun 15, 2023

AntonLydike requested review from tobiasgrosser, superlopuh, JosseVanDelm and PapyChacal June 15, 2023 08:18

AntonLydike self-assigned this Jun 15, 2023

AntonLydike requested review from math-fehr, webmiche and georgebisbas as code owners June 15, 2023 08:18

AntonLydike commented Jun 15, 2023

View reviewed changes

misc: cleanup

60399fd

AntonLydike commented Jun 15, 2023

View reviewed changes

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

AntonLydike added 3 commits June 15, 2023 09:24

misc: make pyright happy

0dd7fd4

add filecheck for print to llvm

c234bc9

move print test to with-mlir folder as it requires mlir

d593ccb

superlopuh requested changes Jun 15, 2023

View reviewed changes

AntonLydike added 2 commits June 15, 2023 09:49

misc: general cleanup and test adding

1bcc71f

print: add more tests for the lowering

56e2145

georgebisbas reviewed Jun 15, 2023

View reviewed changes

JosseVanDelm requested changes Jun 15, 2023

View reviewed changes

AntonLydike added 4 commits June 15, 2023 11:35

dmp: pintf -> printf

514011f

print: fix mlir test

35338fe

print: method name clarification and typo fixes

62e2df2

print: remove unused classes

01af3a2

AntonLydike requested a review from superlopuh June 16, 2023 07:18

AntonLydike requested a review from JosseVanDelm June 16, 2023 07:18

AntonLydike added 2 commits June 16, 2023 08:19

Merge remote-tracking branch 'origin/main' into anton/print-to-println

68b7c50

print: remove outdated comment

d8d037a

superlopuh requested changes Jun 16, 2023

View reviewed changes

superlopuh approved these changes Jun 16, 2023

View reviewed changes

print: add test for symbol name aliasing

2588ebb

webmiche approved these changes Jun 16, 2023

View reviewed changes

AntonLydike added 3 commits June 16, 2023 09:51

print: add test for println without text

f7535b4

print: clarified what legalize_str_for_symbol_name does

cff6692

print: (nfc) more docstring improvements

b6c500a

JosseVanDelm approved these changes Jun 16, 2023

View reviewed changes

AntonLydike merged commit 056a98f into main Jun 16, 2023
12 checks passed

AntonLydike deleted the anton/print-to-println branch June 16, 2023 09:42

cigarichard pushed a commit to cigarichard/xdsl that referenced this pull request Jun 17, 2023

transforms: (print) Add a lowering to printf (xdslproject#1142)

1fd1baa

This adds the `print-to-printf` pass to lower println ops to printf calls

superlopuh mentioned this pull request Jan 22, 2024

documentation: (Toy) add instruction on how to jit Toy using lli #2002

Merged

superlopuh added a commit that referenced this pull request Jan 25, 2024

documentation: (Toy) add instruction on how to jit Toy using lli (#2002)

9d06676

Now that Toy lowers to print_format, we can compile it to LLVM, leveraging the printf flow added in #1142 .

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

transforms: (print) Add a lowering to printf #1142

transforms: (print) Add a lowering to printf #1142

AntonLydike commented Jun 15, 2023

AntonLydike Jun 15, 2023 •

edited

JosseVanDelm Jun 15, 2023

codecov bot commented Jun 15, 2023 •

edited

superlopuh Jun 15, 2023

AntonLydike Jun 15, 2023

superlopuh Jun 15, 2023

AntonLydike Jun 15, 2023

JosseVanDelm Jun 15, 2023

superlopuh Jun 16, 2023

AntonLydike Jun 16, 2023

georgebisbas Jun 15, 2023

superlopuh Jun 15, 2023

AntonLydike Jun 16, 2023

superlopuh Jun 16, 2023

AntonLydike Jun 16, 2023

webmiche Jun 16, 2023

AntonLydike Jun 16, 2023

superlopuh Jun 16, 2023

AntonLydike Jun 16, 2023

AntonLydike Jun 16, 2023

JosseVanDelm left a comment

JosseVanDelm Jun 15, 2023

JosseVanDelm Jun 15, 2023

superlopuh Jun 16, 2023

superlopuh Jun 16, 2023

superlopuh left a comment

webmiche left a comment

AntonLydike commented Jun 16, 2023

JosseVanDelm left a comment

		@@ -0,0 +1,20 @@
		// RUN: xdsl-opt %s -p print-to-printf \| mlir-opt --test-lower-to-llvm \| mlir-translate --mlir-to-llvmir \| filecheck %s
		// this tests straight to llvmir, because I feel that that's a reasonable thing to do.

transforms: (print) Add a lowering to printf #1142

transforms: (print) Add a lowering to printf #1142

Conversation

AntonLydike commented Jun 15, 2023

AntonLydike Jun 15, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 15, 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

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

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

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

Choose a reason for hiding this comment

JosseVanDelm left a comment

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

Choose a reason for hiding this comment

superlopuh left a comment

Choose a reason for hiding this comment

webmiche left a comment

Choose a reason for hiding this comment

AntonLydike commented Jun 16, 2023

JosseVanDelm left a comment

Choose a reason for hiding this comment

AntonLydike Jun 15, 2023 •

edited

codecov bot commented Jun 15, 2023 •

edited