Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

transform: stencil to csl - generate layout module #2720

Draft
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

n-io
Copy link
Collaborator

@n-io n-io commented Jun 13, 2024

Adding a transform from stencil to CSL.

  • Perform initial pass over the stencil.apply ops to figure out stencil pattern and compute grid size (done)
  • Generate CSL layout module (done, except for exported pointers)
  • Generate basic CSL program module (params/imports to setup module structure, translating stencil computation will be done separately)

@n-io n-io added the transformations Changes or adds a transformatio label Jun 13, 2024
@n-io n-io self-assigned this Jun 13, 2024
@n-io n-io requested review from AntonLydike and dk949 June 13, 2024 15:52
Copy link

codecov bot commented Jun 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.97%. Comparing base (454f1b3) to head (5d36d32).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2720      +/-   ##
==========================================
+ Coverage   89.91%   89.97%   +0.05%     
==========================================
  Files         367      368       +1     
  Lines       47302    47440     +138     
  Branches     7229     7245      +16     
==========================================
+ Hits        42533    42684     +151     
+ Misses       3658     3644      -14     
- Partials     1111     1112       +1     

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

@n-io n-io marked this pull request as ready for review June 14, 2024 12:22
@n-io n-io changed the title transform: stencil to csl transform: stencil to csl - generate layout module Jun 14, 2024
Copy link
Collaborator

@AntonLydike AntonLydike left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add comments and docstrings to this PR, it's really hard to follow what you are doing / want to do because of the distinct lack of comments.

ctx.add_params_to_program(
ParamOp.get("stencil_comms_params", ComptimeStructType()),
ParamOp.get("memcpy_params", ComptimeStructType()),
ParamOp.get("z_dim", IntegerType(16, Signedness.SIGNED)),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd recommend not using signedness unless you really need an unsigned integer!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dito for all other integer variables

Copy link
Collaborator Author

@n-io n-io Jun 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd proceed with caution on this - most of what is happening here is setting up module (library) imports, calling module functions, and using that to set params for the pe.csl. It might just be harder to reverse-engineer it in the printer.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the point of view of the printer using signed or signless is exactly the same (they both get printed as signed).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we might need to look into that, some functions have specific requirements and may only accept one of the two

@n-io
Copy link
Collaborator Author

n-io commented Jun 14, 2024

Please add comments and docstrings to this PR, it's really hard to follow what you are doing / want to do because of the distinct lack of comments.

Added comments and doc strings as suggested

@n-io n-io requested a review from AntonLydike June 14, 2024 13:35
Copy link
Collaborator

@AntonLydike AntonLydike left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a great start, but I am not sure if the way we integrate this is the right path right now. I would prefer to have something closer to how GPU dialect does offloading.

I think that would result in much more usable passes and would make our life significantly easier.

I think we need a mid-level intermediate dialect for that though.

xdsl/backend/csl/lowering/convert_stencil_to_csl.py Outdated Show resolved Hide resolved
xdsl/backend/csl/lowering/convert_stencil_to_csl.py Outdated Show resolved Hide resolved
xdsl/backend/csl/lowering/convert_stencil_to_csl.py Outdated Show resolved Hide resolved
xdsl/dialects/csl.py Outdated Show resolved Hide resolved
xdsl/dialects/csl.py Outdated Show resolved Hide resolved
xdsl/dialects/csl.py Outdated Show resolved Hide resolved
@@ -1486,6 +1538,14 @@ class ParamOp(IRDLOperation):

res = result_def(T)

@staticmethod
def get(name: str, result_type: T) -> ParamOp:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

T is not a valid type here, I think? @PapyChacal

xdsl/dialects/csl.py Outdated Show resolved Hide resolved
@n-io
Copy link
Collaborator Author

n-io commented Jun 14, 2024

I think this is a great start, but I am not sure if the way we integrate this is the right path right now. I would prefer to have something closer to how GPU dialect does offloading.

This PR doesn't actually do very much except initialising a layout module and a stub program module that takes care of some administrative tasks such as importing things and initialising the stencil comms library. I'm hoping this should in theory be non-committal as to how we go about further translating.

@n-io n-io requested a review from AntonLydike June 17, 2024 09:30
n-io added a commit that referenced this pull request Jun 17, 2024
Adding constructors for many CSL ops.

This was originally part of #2720

---------

Co-authored-by: n-io <n-io@users.noreply.github.com>
@n-io
Copy link
Collaborator Author

n-io commented Jun 18, 2024

We expect that we will need to generate csl modules very soon and are benching this PR until then.

@n-io n-io marked this pull request as draft June 18, 2024 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
transformations Changes or adds a transformatio
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants