Refactor argument passing #631
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
2 tasks
inducer
force-pushed
the
base-storage-refactor
branch
8 times, most recently
from
570853b
to
700cea5
Compare
inducer
changed the title
base_storage and auto strides and offsets
inducer
force-pushed
the
base-storage-refactor
branch
9 times, most recently
from
66b4532
to
b42e69b
Compare
inducer
force-pushed
the
base-storage-refactor
branch
7 times, most recently
from
7df44a6
to
6cb2ba5
Compare
|
Hi @kaushikcfd, I know that reviewing this is a big ask, and in a way, this PR is already, erm, "too big to fail". :) I'm not sure I feel OK asking you do do a thorough read of this, but if you could give it a rough look, I would be grateful. In general, I think this makes things incrementally more sensible/maintainable, and it does contain a few bug fixes. A slightly earlier version did pass all its tests, and I expect the current version to do so, too. If not, I'll address that. At this point, this is one tangled ball, and I'm not sure there's a point in trying to separate out individual parts. If you feel otherwise, let me know. |
inducer
force-pushed
the
base-storage-refactor
branch
from
6cb2ba5
to
ddd14c7
Compare
- Refactor argument passing so that instead of implicitly-spawned `ImplementedDataInfo` objects, there are actual arguments (for automatic offsets and strides, base storage, and `sep`-tagged arrays). It also centralizes the logic for what goes into argument lists, instead of having various "filtered" versions scattered about. - Get started on type-annotating a bit of loopy. - Switch a not-small number of data structures to be dataclasses, notably `LoopKernel`. - Drop OCCA support from the ISPC target. (I'm not aware of any users, ever.) - Drop the Numba target outright. (I'm not aware of any users, ever.) - Drop `LoopKernel.local_sizes`, which was usable to directly set the workgroup size. (I'm not aware of any users, ever.) - Expire the deprecation for `iname_to_tags`. - Bumps the Python compatibility target to 3.8, for `from __future__ import annotations` and `cached_property` (mypy does not support nested decorators) - Bug fix: `tags` was not part of `LoopKernel.hash_fields` - Bug fix: `InstructionBase.get_write_dependency_names()` was used to find written variables, `InstructionBase.assignee_var_names()` is correct - Bug fix: KernelExecutorBase now uses linearize() so as to not bypass pre-linearization checks (cf. gh-639)
inducer
force-pushed
the
base-storage-refactor
branch
from
ddd14c7
to
ff98acd
Compare
|
Thanks for taking a look! I'll give this a scroll myself later and then proceed with merging. |
|
Scrolled, still looks OK. :) In it goes. |
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
So this got a bit out of control. It now does many things, see the description of the main commit.
base_storageonly once to the device kernel #599.When merging: