backend: (csl) printing for functions, tasks and layout related things #2647

dk949 · 2024-05-27T15:20:58Z

Printing for:

csl.call
csl.task
csl.func
- Merged with csl.task
csl.export
csl.layout

Also introduces a divider between the program and layout, for ease of filechecking and splitting

Extracted printing logic for functions and tasks into one function. If a task can be immediately bound (i.e. it has an ID attribute), it will be. Also added printing for csl.call

This part operates on the program module and prints `@export_symbol`.

This also provides the second half of the symbol exporting logic (i.e. `@export_name` in the layout file).

Not all of these ops and types are implemented in the current printer, but this tests for everything the old branch implemented.

Also test for control tasks

codecov · 2024-05-27T15:26:19Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.68%. Comparing base (3d0acbe) to head (0174108).
Report is 12 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2647      +/-   ##
==========================================
+ Coverage   89.60%   89.68%   +0.07%     
==========================================
  Files         360      361       +1     
  Lines       46172    46319     +147     
  Branches     6974     7012      +38     
==========================================
+ Hits        41373    41540     +167     
+ Misses       3725     3709      -16     
+ Partials     1074     1070       -4

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

AntonLydike · 2024-05-29T10:41:08Z

xdsl/backend/csl/print_csl.py

+    @contextmanager
+    def _in_block(self, block_name: str):
+        self.print(f"{block_name} {{")
+        old_prefix = self._prefix
+        self._prefix += self._INDENT
+        yield
+        self._prefix = old_prefix
+        self.print("}")
+        pass


Does this replace the previous descend method?

Not really, this just wraps a set of prints in <block_name> { ... }. I could combine it with descend, but it's more for cases where we don't have an MLIR Block and need to wrap something in braces.

I see it only being used for comptime regions, if that is the case, maybe don't need a form this generic? Just some food for thought.

It's also used for layout, though I can rewrite that as descend, since layout does have an MLIR Block, then make _in_block _in_comptime instead, since there are no other CSL block types.

Technically, descend can be used to replace all uses of _in_block, but this way felt cleaner, since it's always the same pattern:

<block-name> { <statements> }

What would your thoughts be on:

with self._in_block("comptime") as in_comptime: in_comptime.print(f"@export_symbol({export_val}, {q_name});")

Then we can reuse descend in _in_block and avoid repetition there?

Maybe we can instead replace descend with this instead then? I also feel that the context manager "feels" nicer, I would just argue that we should focus on one method for now.

AntonLydike · 2024-05-29T10:41:43Z

xdsl/backend/csl/print_csl.py

+    def _task_or_fn(
+        self, introducer: str, name: StringAttr, bdy: Region, ftyp: FunctionType
+    ):
+        args = ", ".join(
+            f"{self._get_variable_name_for(arg)} : {self.mlir_type_to_csl_type(arg.type)}"
+            for arg in bdy.block.args
+        )
+        ret = (
+            "void"
+            if len(ftyp.outputs) == 0
+            else self.mlir_type_to_csl_type(ftyp.outputs.data[0])
+        )
+        self.print(f"\n{introducer} {name.data}({args}) {ret} {{")
+        self.descend().print_block(bdy.block)
+        self.print("}")


I would really like to have all functions that print prefixed with print_

AntonLydike · 2024-05-29T10:45:52Z

xdsl/backend/csl/print_csl.py

+    def _bind_task(self, name: str, kind: csl.TaskKind, id: csl.ColorIdAttr | None):
+        if id is None:
+            return
+        with self._in_block("comptime"):
+            self.print(
+                f"@bind_{kind.value}_task({name}, @get_{kind.value}_task_id({self._wrapp_task_id(kind, id.value.data)}));"
+            )


Same here, something like print_task_bind

AntonLydike · 2024-05-29T10:46:47Z

xdsl/backend/csl/print_csl.py

+    def _export_sym_constness(self, ty: FunctionType | csl.PtrType):
+        if isinstance(ty, FunctionType):
+            return None
+        match ty.constness.data:
+            case csl.PtrConst.VAR:
+                return True
+            case csl.PtrConst.CONST:
+                return False
+


I don't quite understand what this function does. Type annotations and docstrings would go a long way in making this more reviewable!

When exporting symbols we have to specify if they are host-writable, except for functions, where it's a compile error to do so.

Will add a docstring and return type

AntonLydike · 2024-05-29T10:48:18Z

xdsl/backend/csl/print_csl.py

+    def _task_or_fn(
+        self, introducer: str, name: StringAttr, bdy: Region, ftyp: FunctionType
+    ):


The introducer could benefit from a rename and better type annotation (e.g. kind: Literal['task', 'fn'])

AntonLydike · 2024-05-29T10:50:32Z

xdsl/backend/csl/print_csl.py

        )


+def _get_layout_program(ops: BlockOps) -> tuple[csl.CslModuleOp, csl.CslModuleOp]:


I think it'd be more appropriate to pass the module here instead of the iterable of it's ops?

Also, docstrings!

dk949 · 2024-05-29T10:50:56Z

@AntonLydike for the print_ prefix, should these still be private (i.e. _print_)?

AntonLydike

I think this is overall a pretty good first printer PR, although I would really really like to see more docstrings here!

AntonLydike · 2024-05-29T10:57:56Z

@AntonLydike for the print_ prefix, should these still be private (i.e. _print_)?

I think they can easily be private as well!

Not a list of it's ops

AntonLydike

Much better already!

AntonLydike · 2024-05-29T14:33:19Z

xdsl/backend/csl/print_csl.py

+        To be used in a `with` statement.
+        Surrounds everything in the `with` with curly braces and .
+
+        This is used instead of descend when


premature end of docstring, I think

AntonLydike · 2024-05-29T14:33:56Z

xdsl/backend/csl/print_csl.py

-        self, introducer: str, name: StringAttr, bdy: Region, ftyp: FunctionType
+    def _print_task_or_fn(
+        self,
+        kind: Literal["fn"] | Literal["task"],


Can be simplified to Literal["fn", "task"]

AntonLydike · 2024-05-29T14:39:15Z

xdsl/backend/csl/print_csl.py

+        self.descend().print_block(bdy.block)
+        self.print("}")
+
+    def _wrapp_task_id(self, kind: csl.TaskKind, id: int) -> str:


nit: typo - should be wrap_task_id

AntonLydike · 2024-05-29T14:41:07Z

xdsl/backend/csl/print_csl.py

-    for mod in prog.body.block.ops:
-        assert isinstance(mod, csl.CslModuleOp)
-        ctx.print_block(mod.body.block)
+    layout, program = _get_layout_program(prog)
+    ctx.print_block(program.body.block)
+    ctx.print(ctx.DIVIDER)
+    ctx.print_block(layout.body.block)


I like the previous approach more of iterating over the modules, I feel this provides more flexibility for e.g. multiple programs or multiple layouts. This feels overly restrictive

We have to print program first as that's when we collect symbols to be exported.

We can allow for multiple programs, but there still needs to be some logic to make sure they all get processed before the one layout.

AntonLydike · 2024-05-30T08:25:38Z

xdsl/backend/csl/print_csl.py

                case anyop:
                    self.print(f"unknown op {anyop}", prefix="//")

-    def descend(self) -> CslPrintContext:
+    @contextmanager
+    def descend(self, block_start: str = ""):


nit: you can add a type annotation here so that we know the type of the context manager:

from typing import ContextManager # ... @contextmanager def descend(self, block_start: str = "") -> ContextManager[CslPrintContext]: ....

I'm curious whether this will work, I would have expected it to rather be -> Iterator[CslPrintContext]

iirc pyright (at least when I tested it) were happy with both, but I find ContextManager to be the most correct imo

When I try ContextManager[CslPrintContext], pyright produces all sorts of errors. ruff also complains that typing.ContextManager is deprecated in favor of AbstractContextManager, AbstractContextManager[CslPrintContext] causes all the same errors, since it's not compatible with Generator.

Iterator[CslPrintContext] works, as does Generator[CslPrintContext, None, None], but I think both of these make the function signature more confusing. Personally, I would leave the return type auto-deduced, and rely on the docstring

Yeah, the type annotation is for the function passed into the decorator, so ContextManager is incorrect

AntonLydike

I think this is in a good-to-go state now!

dk949 · 2024-05-30T11:16:42Z

I think this is in a good-to-go state now!

I forgot to remove the old _in_block method, but I recon it's good now.

dk949 added 8 commits May 27, 2024 14:55

backend: (csl) printing for functions and tasks

4a0fbe2

Extracted printing logic for functions and tasks into one function. If a task can be immediately bound (i.e. it has an ID attribute), it will be. Also added printing for csl.call

backend: (csl) printer for csl.export

782d405

This part operates on the program module and prints `@export_symbol`.

backend: (csl) add printer for csl.layout

4a7021b

This also provides the second half of the symbol exporting logic (i.e. `@export_name` in the layout file).

backend: (csl) divider between layout and program

c95afde

fix: reintroduce removed newline before function def

d127bec

tests: (csl) ported over test from the old branch

4dca9c8

Not all of these ops and types are implemented in the current printer, but this tests for everything the old branch implemented.

tests: (csl) added tests for unbound tasks

b5c8fc2

Also test for control tasks

fix: ssa name indices

1ee9891

dk949 added the backend Compiler backend in xDSL label May 27, 2024

dk949 requested review from superlopuh, AntonLydike and n-io May 27, 2024 15:20

dk949 self-assigned this May 27, 2024

AntonLydike reviewed May 29, 2024

View reviewed changes

AntonLydike requested changes May 29, 2024

View reviewed changes

dk949 added 7 commits May 29, 2024 12:10

added _print_ prefix to printing functions

2ee211b

_get_layout_program should take the module itself

b94a8f9

Not a list of it's ops

better typing and documentation for _print_task_or_fn

91ea7de

documented _get_layout_program

0e4861b

documented _in_block_

ac4c44a

documentation and better typing for _wrapp_task_id

2dfbde7

documentation for _print_bind_task

cbaf24b

documentation and better typing for _memref_global_init

2f72b0a

AntonLydike reviewed May 29, 2024

View reviewed changes

dk949 added 3 commits May 29, 2024 15:41

simplified Literal type

da08a7f

fix: type in function name

fd3bdcc

refactored descend as a context manager

eb27a2f

AntonLydike reviewed May 30, 2024

View reviewed changes

AntonLydike approved these changes May 30, 2024

View reviewed changes

removed unused _in_block method

0174108

AntonLydike merged commit 6cfcb63 into main May 31, 2024
10 checks passed

AntonLydike deleted the david/csl-backend/functions+layout branch May 31, 2024 14:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

backend: (csl) printing for functions, tasks and layout related things #2647

backend: (csl) printing for functions, tasks and layout related things #2647

dk949 commented May 27, 2024

codecov bot commented May 27, 2024 •

edited

Loading

AntonLydike May 29, 2024

dk949 May 29, 2024

AntonLydike May 29, 2024

dk949 May 29, 2024

dk949 May 29, 2024

dk949 May 29, 2024

AntonLydike May 29, 2024

AntonLydike May 29, 2024

AntonLydike May 29, 2024

AntonLydike May 29, 2024

dk949 May 29, 2024

AntonLydike May 29, 2024

AntonLydike May 29, 2024

AntonLydike May 29, 2024

dk949 commented May 29, 2024

AntonLydike left a comment

AntonLydike commented May 29, 2024

AntonLydike left a comment

AntonLydike May 29, 2024

AntonLydike May 29, 2024

AntonLydike May 29, 2024

AntonLydike May 29, 2024

dk949 May 29, 2024

AntonLydike May 30, 2024

superlopuh May 30, 2024

AntonLydike May 30, 2024

dk949 May 30, 2024

superlopuh May 30, 2024

AntonLydike left a comment

dk949 commented May 30, 2024

		)


		def _get_layout_program(ops: BlockOps) -> tuple[csl.CslModuleOp, csl.CslModuleOp]:

backend: (csl) printing for functions, tasks and layout related things #2647

backend: (csl) printing for functions, tasks and layout related things #2647

Conversation

dk949 commented May 27, 2024

codecov bot commented May 27, 2024 • edited Loading

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

dk949 commented May 29, 2024

AntonLydike left a comment

Choose a reason for hiding this comment

AntonLydike commented May 29, 2024

AntonLydike 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

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

AntonLydike left a comment

Choose a reason for hiding this comment

dk949 commented May 30, 2024

codecov bot commented May 27, 2024 •

edited

Loading