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

Fusion arg reduction #602

Closed
wants to merge 16 commits into from
Closed

Fusion arg reduction #602

wants to merge 16 commits into from

Conversation

matthiasdiener
Copy link
Collaborator

@matthiasdiener matthiasdiener commented Apr 25, 2022

⚠️ Not for merge ⚠️

This combines the Fusion actx with #599

cc @kaushikcfd

This fails in the generated Python code:

[...]
 File "/Users/mdiener/Work/efuse/grudge/grudge/op.py", line 604, in _apply_mass_operator
    area_elements = area_element(actx, dcoll, dd=dd_in,
  File "/Users/mdiener/Work/efuse/grudge/grudge/geometry/metrics.py", line 651, in area_element
    return thaw(_area_elements(), actx)
  File "/Users/mdiener/Work/efuse/miniforge3/envs/ceesd/lib/python3.9/site-packages/pytools/__init__.py", line 966, in new_inner
    result = inner(*args)
  File "/Users/mdiener/Work/efuse/grudge/grudge/geometry/metrics.py", line 649, in _area_elements
    return freeze(result, actx)
  File "/Users/mdiener/Work/efuse/miniforge3/envs/ceesd/lib/python3.9/functools.py", line 888, in wrapper
    return dispatch(args[0].__class__)(*args, **kw)
  File "/Users/mdiener/Work/efuse/meshmode/meshmode/dof_array.py", line 389, in _freeze_dofarray
    tuple(ary.array_context.freeze(subary) for subary in ary._data))
  File "/Users/mdiener/Work/efuse/meshmode/meshmode/dof_array.py", line 389, in <genexpr>
    tuple(ary.array_context.freeze(subary) for subary in ary._data))
  File "/Users/mdiener/Work/efuse/arraycontext/arraycontext/impl/pytato/__init__.py", line 295, in freeze
    evt, out_dict = pt_prg(self.queue, **bound_arguments)
  File "/Users/mdiener/Work/efuse/pytato/pytato/target/loopy/__init__.py", line 146, in __call__
    return self.program(queue,
  File "/Users/mdiener/Work/efuse/loopy/loopy/translation_unit.py", line 347, in __call__
    return pex(*args, **kwargs)
  File "/Users/mdiener/Work/efuse/loopy/loopy/target/pyopencl_execution.py", line 371, in __call__
    return translation_unit_info.invoker(
  File "/Users/mdiener/Work/efuse/miniforge3/envs/ceesd/lib/python3.9/site-packages/pytools/py_codegen.py", line 150, in __call__
    return self.func(*args, **kwargs)
  File "<generated code for 'invoke__pt_kernel_loopy_kernel'>", line 1204, in invoke__pt_kernel_loopy_kernel
  File "<generated code for 'invoke__pt_kernel_loopy_kernel'>", line 58, in _lpy_host__pt_kernel
AssertionError

Comment on lines 897 to 904
idis = self.preprocess_idis(
kernel, codegen_result.get_idis_for_subkernel(kernel, subkernel))

return FunctionDeclarationWrapper(
FunctionDeclaration(
name,
[self.idi_to_cgen_declarator(codegen_state.kernel, idi)
for idi in self.preprocess_idis(
codegen_state.kernel,
codegen_state.implemented_data_info)]
[self.idi_to_cgen_declarator(kernel, idi)
for idi in idis]
Copy link
Owner

Choose a reason for hiding this comment

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

What difference does this make? If it's important, explain in a comment!

Copy link
Collaborator Author

@matthiasdiener matthiasdiener Apr 27, 2022

Choose a reason for hiding this comment

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

I'm not sure what you mean - the previous code was incorrect (the arguments to preprocess_idis). I restructured this a little to reduce the forest of parentheses ;-)

Copy link
Owner

Choose a reason for hiding this comment

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

Hm. I see the change now. One thing that we may have to negotiate is that I find the "forest of parentheses" version easier to reason about. (The value is unbound and therefore I can tell that it won't be used/changed below.)

@inducer
Copy link
Owner

inducer commented Apr 28, 2022

Looks like db00b36 is the net change here. @kaushikcfd, would it make sense for you to grab that change?

@kaushikcfd
Copy link
Collaborator

Looks like db00b36 is the net change here. @kaushikcfd, would it make sense for you to grab that change?

Yep, I'll cherry-pick this!

kaushikcfd and others added 4 commits May 6, 2022 12:14
Co-authored-by: Matthias Diener <matthias.diener@gmail.com>
Co-authored-by: Matthias Diener <matthias.diener@gmail.com>
@matthiasdiener
Copy link
Collaborator Author

matthiasdiener commented Jul 1, 2022

Closing this since #599 has been closed (see #631 for the new version).

@matthiasdiener matthiasdiener deleted the fusion_arg_reduction branch July 1, 2022 00:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants