-
Notifications
You must be signed in to change notification settings - Fork 33
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
Expand from write nodes inside reductions #395
Conversation
555ed06
to
0e646d2
Compare
I actually signed the commits with my ssh key, but it looks like it's not accepting that. |
I'm surprised the vscode workspace wasn't already in there, as I think that's the typical place to put it. I think it's generally useful to have a place to put output files (and the MLIR dumps should probably go there by default). I can drop these though. Signed-off-by: Geoffrey Martin-Noble <[email protected]>
Right now, it only looks at write nodes in the root graph and misses ones in reductions. Instead we walk all the regions for `Write` nodes and `GetResult` nodes without users. `get_leaf_node_types` also appeared to always return `[Write]` as it sets `leaf_nodes` to that and then immediately checks if it's empty, which it never will be, so I deleted it. Signed-off-by: Geoffrey Martin-Noble <[email protected]>
0e646d2
to
4b133fd
Compare
@harsh-nod PTAL |
DCO allows signing (instead of sign-off) for organization members. I've sent you an invite to iree-org + the iree-triage team. That will also let checks run without approval. IREE docs: https://iree.dev/developers/general/contributing/#developer-certificate-of-origin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, this looks good! Could you add a lit test with a write inside a reduction in expansion to make sure that this works properly?
Added tests. PTAL |
Confirmed this fails on main and succeeds with my change.
The thing I was doing here actually failed because apparently _node_list isn't an iterator, so switched to looking at `_root.prev`, althoug I'm a bit leery of using a private member here.
02881c4
to
c6532d6
Compare
The failing test doesn't seem related to my change and passes on my local mi210. It's also a numerical diff on a check that already has rtol set to 0.8, which suggests to me that it's already broken and should probably be xfailed or skipped... |
Yes we have flaky attention tests that need to be debugged, but you can just retrigger for now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm! thanks!
Right now, it only looks at write nodes in the root graph and misses ones in reductions. Instead we walk all the regions for
Write
nodes andGetResult
nodes without users.get_leaf_node_types
also appeared to always return[Write]
as it setsleaf_nodes
to that and then immediately checks if it's empty, which it never will be, so I deleted it.Also includes some .gitignore additions that I can drop if you don't like them.
Fixes #374