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

[IR] Pretty-printer for LogicalObjectFifoFromMemrefOp #1017

Merged
merged 1 commit into from
Jan 13, 2025

Conversation

newling
Copy link
Contributor

@newling newling commented Jan 9, 2025

Makes IR easier to mentally parse -- it should be easier to figure out which columns/rows a LogicalObjectFifoFromMemrefOp is on.

%lof_3_2 = ... // column 3, row 2
%lof_3_r = ... // column 3, multiple rows or unkown row 

The lit tests have more examples

Copy link
Contributor

@Abhishek-Varma Abhishek-Varma left a comment

Choose a reason for hiding this comment

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

Nice addition!

I have a naming related request if it's okay - since we have tile assignment related pass/functionality, perhaps we make a difference (naming-wise) between a LOF that :-

  1. has not been assigned any tile.
  2. has been assigned with unknown tiles/tiles on different row and col.

memref<8xi32> -> !amdaie.logicalobjectfifo<memref<8xi32>>

// logicalobjectfifo with a tile with a known column, unkown row:
// CHECK: %lof_2_r_6 =
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd expect this to be %lof_2_r_0 or %lof_2_r_1 or %lof_2_r_2 instead %lof_2_r_6 - not sure where the last _6 is coming from.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me explain and then improve!
Explanation: there is a %tile_0 and a %tile_1 before %lof_2_r_2, that's why it's starting at 2 not 0.
Improvement TODO: (1) tiles can have better suffixing (2) separate into 2+ lit tests.

memref<8xi32> -> !amdaie.logicalobjectfifo<memref<8xi32>>

// logicalobjectfifo with a single unknown tile:
// CHECK: %lof_2 =
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly I'd expect this as %lof_0 or %lof_1 ?

@newling
Copy link
Contributor Author

newling commented Jan 10, 2025

I have a naming related request if it's okay - since we have tile assignment related pass/functionality, perhaps we make a difference (naming-wise) between a LOF that :-

  1. has not been assigned any tile.
  2. has been assigned with unknown tiles/tiles on different row and col.

Ok. I guess that would mean something like %lof for (1) and %lof_c_r for (2). I'll try this

Copy link
Collaborator

@jtuyls jtuyls left a comment

Choose a reason for hiding this comment

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

This is great!

@newling newling force-pushed the from_memref_ir_sugar branch from af5474c to 672e5b3 Compare January 13, 2025 17:04
Copy link
Contributor

@Abhishek-Varma Abhishek-Varma left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @newling !

@newling newling enabled auto-merge (squash) January 13, 2025 17:24
@newling newling merged commit 6d4d3ea into nod-ai:main Jan 13, 2025
6 checks passed
Abhishek-Varma pushed a commit that referenced this pull request Jan 13, 2025
Will now be `%tile_c_3` and `%tile_2_r` if one or other of the column
and row are integers. As opposed to just `%tile` and `%tile_1`. This
aligns tile printing with the [printing of
logicalobjectfifos](#1017).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants