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

dialects: Add NoMemoryEffects to memref and snitch_runtime dialect ops #2706

Merged
merged 3 commits into from
Jun 11, 2024

Conversation

AntonLydike
Copy link
Collaborator

This PR adds NoMemoryEffects trait to ops from the memref and snitch_runtime dialect that should have them.

@AntonLydike AntonLydike added the dialects Changes on the dialects label Jun 11, 2024
@AntonLydike AntonLydike self-assigned this Jun 11, 2024
@AntonLydike AntonLydike force-pushed the anton/add-no-effects-to-things branch from 27bbb7b to 04e0695 Compare June 11, 2024 15:29
Copy link
Collaborator

@PapyChacal PapyChacal left a comment

Choose a reason for hiding this comment

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

I have doubts regarding the snitch ones, but giving approval if given my comments it's still considered right, as I don't know the dialect's contract or semantics.

xdsl/dialects/memref.py Show resolved Hide resolved
xdsl/dialects/snitch_runtime.py Show resolved Hide resolved
@AntonLydike
Copy link
Collaborator Author

@PapyChacal any idea what's wrong in the memref canonicalization test? It seems fishy to me.

@PapyChacal
Copy link
Collaborator

@PapyChacal any idea what's wrong in the memref canonicalization test? It seems fishy to me.

What sounds fishy to you? You rightfully marked memref.subview as having no side effect, so canonicalization will safely erase any unused subview 🙂

@AntonLydike
Copy link
Collaborator Author

AntonLydike commented Jun 11, 2024

@PapyChacal any idea what's wrong in the memref canonicalization test? It seems fishy to me.

What sounds fishy to you? You rightfully marked memref.subview as having no side effect, so canonicalization will safely erase any unused subview 🙂

I think your eyes deceive you:

I added a use to the memref, and so now we basically have:

func A(x: memref) {
 y = cast(cast(x))
 opaque_op(y)
}

And xdsl-opt -p canonicalize folds the two casts into one. But the filecheck expects there to be two casts!

@AntonLydike AntonLydike force-pushed the anton/add-no-effects-to-things branch from fecc613 to 8b22eb0 Compare June 11, 2024 15:56
@AntonLydike
Copy link
Collaborator Author

AntonLydike commented Jun 11, 2024

Ahh, I figured it out!

The memref canonicalization pass did not remove the old cast, it basically transformed the IR to:

func a(x: memref) {
y = old_cast(x)
y_new = new_cast(x)
 opaque_op(y_new)
}

and cse then righfully garbage collected the first cast!

Copy link

codecov bot commented Jun 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.86%. Comparing base (31cbdd2) to head (8b22eb0).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2706   +/-   ##
=======================================
  Coverage   89.86%   89.86%           
=======================================
  Files         367      367           
  Lines       47401    47412   +11     
  Branches     7223     7223           
=======================================
+ Hits        42598    42608   +10     
  Misses       3694     3694           
- Partials     1109     1110    +1     

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

@AntonLydike AntonLydike merged commit 43a625a into main Jun 11, 2024
10 checks passed
@AntonLydike AntonLydike deleted the anton/add-no-effects-to-things branch June 11, 2024 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dialects Changes on the dialects
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants