Skip to content

Apply builder pattern to kernel launcher #1243

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

Merged
merged 2 commits into from
Dec 14, 2023

Conversation

ZzEeKkAa
Copy link
Contributor

Update kernel launcher builder to use builder pattern. Since the argument list is long we need modular way of providing those arguments to make code more readable and extendable.

  • Have you provided a meaningful PR description?
  • Have you added a test, reproducer or referred to an issue with a reproducer?
  • Have you tested your changes locally for CPU and GPU devices?
  • Have you made sure that new changes do not introduce compiler warnings?
  • If this PR is a work in progress, are you filing the PR as a draft?

@ZzEeKkAa ZzEeKkAa self-assigned this Dec 13, 2023
@ZzEeKkAa ZzEeKkAa requested a review from diptorupd as a code owner December 13, 2023 18:32
@ZzEeKkAa ZzEeKkAa force-pushed the feature/apply_builder_pattern_to_kernel_launcher branch from 12dbc5a to 439b5f9 Compare December 13, 2023 21:25
@ZzEeKkAa ZzEeKkAa force-pushed the feature/apply_builder_pattern_to_kernel_launcher branch from 439b5f9 to 3be676b Compare December 14, 2023 02:13
@diptorupd
Copy link
Contributor

diptorupd commented Dec 14, 2023

Few dead code that I noticed after your changes in parfor_lowerer.py

  • _KernelArgs
  • from numba_dpex.core.datamodel.models import dpex_data_model_manager as dpex_dmm

@ZzEeKkAa ZzEeKkAa force-pushed the feature/apply_builder_pattern_to_kernel_launcher branch from 3be676b to b8321fb Compare December 14, 2023 02:13
Copy link
Contributor

@diptorupd diptorupd left a comment

Choose a reason for hiding this comment

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

Nice clean up. Makes the code readable and reduces redundancies.

@@ -11,6 +17,48 @@
from numba_dpex.dpctl_iface import libsyclinterface_bindings as sycl
from numba_dpex.dpctl_iface._helpers import numba_type_to_dpctl_typenum

MAX_SIZE_OF_SYCL_RANGE = 3
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a place in parfor_lowerer.py where the same hard coing is done. It will be goo to move the global to dpctl_iface and use it from there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess I've already removed it from parfor_lowerer. This constant is beeing used there through the set_range() call.

@ZzEeKkAa
Copy link
Contributor Author

ZzEeKkAa commented Dec 14, 2023

There is an issue in this change. Before this change parfor_lowerer was using from numba_dpex.core.datamodel.models import dpex_data_model_manager as dpex_dmm data model manager, which is the same as kernel data model manager, but now it uses data model manager from context which is equal to numba's data model manager. It is okay as long as number of arguments for USMNdArray and DpnpNdArray are the same, but I would introduce KernelLaunchIRBuilder.kernel_data_model_manager attribute to store the right dmm on instance creation for use with methods that flattened arguments.

@ZzEeKkAa ZzEeKkAa force-pushed the feature/apply_builder_pattern_to_kernel_launcher branch 2 times, most recently from d00701f to 7d1f4b8 Compare December 14, 2023 15:29
@ZzEeKkAa ZzEeKkAa force-pushed the feature/apply_builder_pattern_to_kernel_launcher branch from 7d1f4b8 to 649ca87 Compare December 14, 2023 15:39
@ZzEeKkAa ZzEeKkAa enabled auto-merge December 14, 2023 15:40
@ZzEeKkAa ZzEeKkAa merged commit 2d6ddb8 into main Dec 14, 2023
@ZzEeKkAa ZzEeKkAa deleted the feature/apply_builder_pattern_to_kernel_launcher branch December 14, 2023 16:09
github-actions bot added a commit that referenced this pull request Dec 14, 2023
…rn_to_kernel_launcher

Apply builder pattern to kernel launcher 2d6ddb8
github-actions bot added a commit to chudur-budur/numba-dpex that referenced this pull request Dec 14, 2023
…ilder_pattern_to_kernel_launcher

Apply builder pattern to kernel launcher 2d6ddb8
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.

2 participants