Implement SplitPytatoArrayContext that attempts a trivial parallelization strategy #216
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
Collaborator
kaushikcfd
commented
Jan 17, 2023
•
kaushikcfd
commented
- CI should pass once Fix codegen for temporary variables declaration loopy#738 is in.
-
⚠️ Concurrency across reductions is not targeted i.e. results in poor performance for computing error norms, etc. (Maybe we should fix this before merging?)- In the meshmode version, we desperately froze such reductions eagerly
- Update: Added a transformation specifically for "reduce-to-scalar" operations to parallelize the reduction operation using loopy transformations.
- We need a better name than SplitPytatoArrayContext.
kaushikcfd
force-pushed
the
split_pytato_actx
branch
5 times, most recently
from
615e2b5
to
5249a38
Compare
|
Making this a draft until the CI is fixed. |
kaushikcfd
force-pushed
the
split_pytato_actx
branch
3 times, most recently
from
adf8d2b
to
b736f3a
Compare
1 task
kaushikcfd
force-pushed
the
split_pytato_actx
branch
3 times, most recently
from
11583d7
to
fe7517e
Compare
7 tasks
kaushikcfd
force-pushed
the
split_pytato_actx
branch
3 times, most recently
from
be0878b
to
ac39271
Compare
kaushikcfd
force-pushed
the
split_pytato_actx
branch
2 times, most recently
from
41ecb8d
to
bb03c86
Compare
inducer
force-pushed
the
split_pytato_actx
branch
from
bb03c86
to
b8c991a
Compare
inducer
reviewed
May 18, 2023
| @@ -714,7 +716,7 @@ def test_array_equal(actx_factory): | |||
| def test_array_context_einsum_array_manipulation(actx_factory, spec): | |||
| actx = actx_factory() | |||
|
|
|||
| mat = actx.from_numpy(np.random.randn(10, 10)) | |||
| mat = actx.from_numpy(np.random.randn(16, 16)) | |||
There was a problem hiding this comment.
Is there a specific reason why these have to be a nice power of two now?
| import pytato | ||
|
|
||
|
|
||
| class SplitPytatoPyOpenCLArrayContext(PytatoPyOpenCLArrayContext): |
There was a problem hiding this comment.
I'm not sure why this thing is named "split". Maybe "generic parallelizing"/"basic parallelizing"?
Comment on lines
+3
to
+5
| Copyright (C) 2023 Andreas Kloeckner | ||
| Copyright (C) 2022 Matthias Diener | ||
| Copyright (C) 2022 Matt Smith |
There was a problem hiding this comment.
All these (other than @kaushikcfd) should be U of I BOT. None of us hold copyright to our work.
| @@ -0,0 +1,143 @@ | |||
| """ | |||
Comment on lines
+91
to
+93
| We deliberately avoid using :class:`pytato.transform.CombineMapper` since | ||
| the mapper's caching structure would still lead to recomputing | ||
| the union of sets for the results of a revisited node. |
There was a problem hiding this comment.
Not sure I understand this comment. Could you explain? And: shouldn't we fix this in CombineMapper (if we can)? (or at least file an issue in pytato?) (also above)
|
|
||
| def _split_reduce_to_scalar_across_work_items( | ||
| kernel: lp.LoopKernel, | ||
| callables: Mapping[str, InKernelCallable], |
There was a problem hiding this comment.
A bit weird that callables is needed here/by precompute_for_single_kernel.
| device: "pyopencl.Device", | ||
| ) -> lp.LoopKernel: | ||
|
|
||
| assert len({kernel.id_to_insn[insn_id].reduction_inames() |
There was a problem hiding this comment.
Could elevate reduction inames to a property of the _LoopNest?
|
|
||
| # collect loop nests of instructions that assign to scalars in the array | ||
| # program. | ||
| insn_id_to_loop_nest: Mapping[str, _LoopNest] = { |
There was a problem hiding this comment.
If insn_id_to_loop_nest isn't used, why not go straight to all_loop_nests?
Comment on lines
+443
to
+444
| This routine **assumes** that the entrypoint in *t_unit* global | ||
| barriers inserted as per :func:`_get_call_kernel_insn_ids`. |
There was a problem hiding this comment.
Grammar? I'm not sure I understand.
Comment on lines
+497
to
+499
| # a mapping from shape to the available base storages from temp variables | ||
| # that were dead. | ||
| shape_to_available_base_storage: Dict[int, Set[str]] = defaultdict(set) |
There was a problem hiding this comment.
Suggested change
| # a mapping from shape to the available base storages from temp variables | |
| # that were dead. | |
| shape_to_available_base_storage: Dict[int, Set[str]] = defaultdict(set) | |
| # a mapping from size in bytes to the available base storages from temp variables | |
| # that were dead. | |
| nbytes_to_available_base_storage: Dict[int, Set[str]] = defaultdict(set) |
Otherwise Intel OpenCL gets its integer arithmetic wrong.
kaushikcfd
force-pushed
the
split_pytato_actx
branch
from
b8c991a
to
b4488ba
Compare
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.