(dialect+transform): stencil to new csl_stencil dialect and transform

[n-io]

Introduces an intermediate dialect with ops:

• csl_stencil.prefetch to indicate prefetched buffer transfers
• csl_stencil.access performs a stencil.access to a prefetched buffer

The stencil-to-csl-stencil transform:

• lowers dmp.swap to csl_stencil.prefetch
• adds prefetched buffers to signature of stencil.apply
• lowers stencil.access to csl_stencil.access iff they are accesses to prefetched buffers

For a more detailed description see the document in #2747. This PR implements Step 1 outlined there.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.79%. Comparing base (0f6558a) to head (93f2e05).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2766      +/-   ##
==========================================
- Coverage   89.81%   89.79%   -0.03%     
==========================================
  Files         372      375       +3     
  Lines       47817    48012     +195     
  Branches     7331     7362      +31     
==========================================
+ Hits        42945    43110     +165     
- Misses       3736     3752      +16     
- Partials     1136     1150      +14     

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

[n-io]

I'd welcome suggestions on file structure/layout.

As for the dialect, riscv takes the approach of having several riscv_*.py files in xdsl/dialects, while x86 has its own subdir. A subdir named csl would clash with the current csl.py, so I wanted to get feedback on how to structure that and where to place the intermediate dialect file (to be clear, the current csl_ dir name is not supposed to be committed, only for the purposes of reviewing). Alternatively, it can go into experimental.

As for the transform, it is currently placed in transform/experimental. Alternatively, it can also be moved to transform or to backend/csl/lowering. I'd tend towards the latter, while wondering if it has any impact on where to place the dialect.

[PapyChacal]

Minor comment but LGTM generally!

On the file layout situation, I would be in favor of a csl directory containing a nested dialect file. Nothing enforces it to be directly in xdsl/dialects.
If you're concerned about the module path, i.e from xdsl.dialects.csl.csl import CSL, you could, in xdsl/dialects/csl/__init__.py import the dialect to expose it from there, enabling from xdsl.dialects.csl import CSL event if it is nested.
Note: I would personally not like xdsl/dialects/csl/__init__.py to contain the dialect definition itself though.
All of this being in answer to your mention of the topic; I wouldn't be against merging for any file layout point myself!

[AntonLydike]

I think I have conceptual questions on both the csl_stencil dialect and the rewrite, maybe it makes sense to split the two off?

Also, this PR is severely lacking comments and docstrings. A lot of things are difficult to review for me, as I have to reconstruct what the method/op/rewrite/pass does from the code. This makes the review error prone as well, as I am never sure if you intended something or if it's a bug...

[n-io]

Minor comment but LGTM generally!

On the file layout situation, I would be in favor of a csl directory containing a nested dialect file. Nothing enforces it to be directly in xdsl/dialects. If you're concerned about the module path, i.e from xdsl.dialects.csl.csl import CSL, you could, in xdsl/dialects/csl/__init__.py import the dialect to expose it from there, enabling from xdsl.dialects.csl import CSL event if it is nested. Note: I would personally not like xdsl/dialects/csl/__init__.py to contain the dialect definition itself though. All of this being in answer to your mention of the topic; I wouldn't be against merging for any file layout point myself!

@PapyChacal I believe having an __init__.py with something like

from .csl import *

__all__ = ["csl"]

will unfortunately result in an F405 error. Not entirely sure how to go about it.

[PapyChacal]

Minor comment but LGTM generally!
On the file layout situation, I would be in favor of a csl directory containing a nested dialect file. Nothing enforces it to be directly in xdsl/dialects. If you're concerned about the module path, i.e from xdsl.dialects.csl.csl import CSL, you could, in xdsl/dialects/csl/__init__.py import the dialect to expose it from there, enabling from xdsl.dialects.csl import CSL event if it is nested. Note: I would personally not like xdsl/dialects/csl/__init__.py to contain the dialect definition itself though. All of this being in answer to your mention of the topic; I wouldn't be against merging for any file layout point myself!

@PapyChacal I believe having an __init__.py with something like

from .csl import *

__all__ = ["csl"]

will unfortunately result in an F405 error. Not entirely sure how to go about it.

We do this in a few places already, you could just add # noqa: TID251 on that line

Or list explicitly what you want to expose

[AntonLydike]

Hmm, I think it might instead be the relative part not the star part of the import that's the problem here, no?

On June 24, 2024 12:28:38 PM GMT+01:00, Emilien Bauer ***@***.***> wrote: > > Minor comment but LGTM generally! > > On the file layout situation, I would be in favor of a `csl` directory containing a nested dialect file. Nothing enforces it to be directly in `xdsl/dialects`. If you're concerned about the module path, i.e `from xdsl.dialects.csl.csl import CSL`, you could, in `xdsl/dialects/csl/__init__.py` import the dialect to expose it from there, enabling `from xdsl.dialects.csl import CSL` event if it is nested. Note: I would personally not _like_ `xdsl/dialects/csl/__init__.py` to contain the dialect definition itself though. All of this being in answer to your mention of the topic; I wouldn't be against merging for any file layout point myself! > > @PapyChacal I believe having an `__init__.py` with something like > > ``` > from .csl import * > > __all__ = ["csl"] > ``` > > will unfortunately result in an [F405](https://docs.astral.sh/ruff/rules/undefined-local-with-import-star-usage/) error. Not entirely sure how to go about it. We do this in a few places already, you could just add `# noqa: TID251` on that line Or list explicitly what you want to expose -- Reply to this email directly or view it on GitHub: #2766 (comment) You are receiving this because your review was requested. Message ID: ***@***.***>

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

[AntonLydike]

Added more code-level comments to the rewrite itself

[AntonLydike]

🚀