-
Notifications
You must be signed in to change notification settings - Fork 1
yield before wait/reserve and support for matrix multiplication #96
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
Conversation
e20d9b9 to
8f18e74
Compare
test/sim/test_program.py
Outdated
| tt_testing.assert_close(out, expected) | ||
|
|
||
| def test_modes_produce_same_result(self) -> None: | ||
| """Test that THREADED mode works (COOPERATIVE skipped due to getsource issues).""" |
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.
What's the getsource issue?
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.
ah, just some issue I had while working on this, fixed now to run on both modes
python/sim/decorators.py
Outdated
| def decorator(func: FunctionType) -> BindableTemplate: | ||
| class ComputeTemplate: | ||
| __name__ = func.__name__ | ||
| _func = func # Store original function for inspect.getsource() |
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.
Consider using __wrapped__ instead of _func here. It's the standard convention from functools.wraps and has built-in support via inspect.unwrap() which would simplify the cooperative mode code to just inspect.unwrap(func) instead of manually accessing _func. Also means tools like debuggers and other decorators automatically understand the wrapping chain.
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.
Maybe this is the issue I commented on earlier regarding getsource?
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.
yes, exactly! Thanks, I fixed it now :)
| 1 if i < remainder_tiles else 0 | ||
| ) | ||
| core_work.append((start_tile_id, num_tiles_this_core)) | ||
| start_tile_id += num_tiles_this_core |
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.
After #67 goes in this will be available in our repo with no dependencies
zoecarver
left a comment
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.
Do we have some simulator documentation? Might be nice to document the new execution modes outside of the code. This looks good to me!
| @ttl.compute() | ||
| def mm_compute(): | ||
| # Compute each output tile (m, n). | ||
| for m in range(Mt): |
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.
nit unused args
| b_block = b_cb.wait() # blocking | ||
|
|
||
| if k == 0: | ||
| # Initialize output tile |
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.
shouldn't need initialization/first iteration logic here
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.
See below
| b_block = b_cb.wait() # blocking | ||
|
|
||
| if k == 0: | ||
| # Initialize output tile. |
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.
same thing, shouldn't need first iteration logic
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.
This turns out to be a rather involved change and I'd prefer to fix it in another PR. In the meantime I've created an issue for this:
I've created a new ticket for that: |
Uh oh!
There was an error while loading. Please reload this page.