Skip to content

add an example for combination of Disjoint Pool and L0 #384

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 11, 2024
Merged

add an example for combination of Disjoint Pool and L0 #384

merged 1 commit into from
Apr 11, 2024

Conversation

bratpiorka
Copy link
Contributor

@bratpiorka bratpiorka commented Mar 25, 2024 •
edited
Loading

Description

Add an example for combination of Disjoint Pool and L0. NOTE: it would be currently build only on Linux and executed on systems with Level-Zero supported device

Checklist

  • Code compiles without errors locally
  • All tests pass locally
  • CI workflows execute properly
  • CI workflows, not executed per PR (e.g. Nightly), execute properly
  • Added/extended example(s) to cover this functionality
  • All newly added source files have a license
  • All newly added source files are referenced in CMake files

TODO: web documentation will be created in separate PR

@bratpiorka bratpiorka force-pushed the rrudnick_dis_l0_example branch 10 times, most recently from b58285c to ab608f8 Compare March 29, 2024 15:35
@bratpiorka bratpiorka force-pushed the rrudnick_dis_l0_example branch 2 times, most recently from ba63cea to 97a3d3d Compare April 2, 2024 14:32
lplewa
lplewa reviewed Apr 2, 2024
View reviewed changes
@bratpiorka bratpiorka force-pushed the rrudnick_dis_l0_example branch from 97a3d3d to 817e95d Compare April 2, 2024 14:38
bratpiorka
bratpiorka commented Apr 2, 2024
View reviewed changes
lplewa
lplewa requested changes Apr 2, 2024
View reviewed changes
Copy link
Contributor

@lplewa lplewa left a comment

Choose a reason for hiding this comment

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

Please print all error messages to stderr(fprintf) also ensure that you include new line in it.

PatKamin
PatKamin reviewed Apr 3, 2024
View reviewed changes
@bratpiorka bratpiorka force-pushed the rrudnick_dis_l0_example branch 4 times, most recently from 02103c0 to 721aab4 Compare April 3, 2024 08:52
lplewa
lplewa reviewed Apr 3, 2024
View reviewed changes
lplewa
lplewa requested changes Apr 4, 2024
View reviewed changes
Copy link
Contributor

@lplewa lplewa left a comment

Choose a reason for hiding this comment

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

i tried to build this example on my side:

/usr/bin/ld: cannot find -lze_loader: No such file or directory collect2: error: ld returned 1 exit status

  1. this problem should be detected during configure step.
  2. example do not include any information about dependencies needed.

examples/CMakeLists.txt
"Basic example requires UMF_BUILD_LIBUMF_POOL_SCALABLE and UMF_ENABLE_POOL_TRACKING
to be turned ON - skipping")
"GPU shared memory example requires "
"UMF_BUILD_LEVEL_ZERO_PROVIDER and UMF_BUILD_LIBUMF_POOL_DISJOINT "
Copy link
Contributor

Choose a reason for hiding this comment

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

This message is incorrect. Please fix it to include build GPU examples variable.

(BTW do we really need "UMF_BUILD_GPU_EXAMPLES" variable - imho if all dependencies are ON, we should build it by default)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link
Contributor

Choose a reason for hiding this comment

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

I second the question - do we need extra UMF_BUILD_GPU_EXAMPLES option? We could enable testing this example if UMF_BUILD_GPU_TESTS is ON.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Testing (what we actually run) has nothing to do with the build options. UMF_BUILD_GPU_TESTS is used for building, not testing. If you want to extend this flag, rename it UMF_BUILD_GPU_TESTS_AND_EXAMPLES or UMG_BUILD_GPU_CODE etc. I agree that we could do some cleanup in build flags - could we address this in additional PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

What I'm saying, is we could (as Łukasz stated above) build it all the time, as long as L0_PROVIDER is ON. We don't need this new flag just for building.

Nontheless, I guess, we could clean flags separately later on...

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we need a dedicated cmake option for GPU examples (UMF_BUILD_GPU_EXAMPLES). After the #427 is implemented we will have examples that will be installed with a pre-built version of UMF. So customer needs an option to disable/enable GPU examples.

Copy link
Contributor

@PatKamin PatKamin Apr 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think that this extra flag is not needed as when it is turned ON but the two other flags are not, nothing happens. I think that existing flag UMF_BUILD_EXAMPLES suffices. If the user wants to have examples build, then all examples for a given config options set will be built.

As it is now, the user would have to set both UMF_BUILD_EXAMPLES and UMF_BUILD_GPU_EXAMPLES flags to build this example. Seeing these two options in CMake I would expect that only the second one is needed for building the GPU examples.

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 think that all changes around cmake files, flags and folders could be done as a part of #427

@vinser52 vinser52 mentioned this pull request Apr 8, 2024
Merged
6 tasks
vinser52
vinser52 reviewed Apr 8, 2024
View reviewed changes
examples/basic/gpu_shared_memory.c Outdated

// This function initializes the Level Zero driver and creates a context in the
// first GPU device found.
int level_zero_init(ze_driver_handle_t *driver, ze_device_handle_t *device,
Copy link
Contributor

Choose a reason for hiding this comment

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

In PR #418 I created some utility functions on top of the L0 API. We need to coordinate and decide how to re-use them among examples and tests where L0 provider is involved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vinser52 I used you file - please rebase after my changes have been added

@bratpiorka bratpiorka force-pushed the rrudnick_dis_l0_example branch from 721aab4 to f8e27f8 Compare April 9, 2024 08:55
vinser52
vinser52 reviewed Apr 9, 2024
View reviewed changes
@bratpiorka bratpiorka force-pushed the rrudnick_dis_l0_example branch 2 times, most recently from cdfc7c5 to 909a17c Compare April 9, 2024 12:45
@bratpiorka bratpiorka force-pushed the rrudnick_dis_l0_example branch from 909a17c to ebb8ea1 Compare April 9, 2024 14:07
lukaszstolarczuk
lukaszstolarczuk reviewed Apr 9, 2024
View reviewed changes
examples/CMakeLists.txt
"Basic example requires UMF_BUILD_LIBUMF_POOL_SCALABLE and UMF_ENABLE_POOL_TRACKING
to be turned ON - skipping")
"GPU shared memory example requires "
"UMF_BUILD_LEVEL_ZERO_PROVIDER and UMF_BUILD_LIBUMF_POOL_DISJOINT "
Copy link
Contributor

Choose a reason for hiding this comment

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

I second the question - do we need extra UMF_BUILD_GPU_EXAMPLES option? We could enable testing this example if UMF_BUILD_GPU_TESTS is ON.

@bratpiorka bratpiorka force-pushed the rrudnick_dis_l0_example branch 2 times, most recently from 17f0510 to dbbf2b0 Compare April 10, 2024 08:40
@bratpiorka bratpiorka requested a review from lplewa April 10, 2024 08:50
@bratpiorka bratpiorka marked this pull request as ready for review April 10, 2024 08:50
@bratpiorka bratpiorka requested a review from a team as a code owner April 10, 2024 08:50
@bratpiorka
Copy link
Contributor Author

please re-review. Web documentation will be added in separate PR

lukaszstolarczuk
lukaszstolarczuk reviewed Apr 10, 2024
View reviewed changes
@bratpiorka bratpiorka force-pushed the rrudnick_dis_l0_example branch from dbbf2b0 to 95b9692 Compare April 10, 2024 11:40
lplewa
lplewa requested changes Apr 10, 2024
View reviewed changes
@bratpiorka bratpiorka force-pushed the rrudnick_dis_l0_example branch from 95b9692 to f092d2e Compare April 10, 2024 15:18
@bratpiorka bratpiorka force-pushed the rrudnick_dis_l0_example branch from f092d2e to 926678b Compare April 11, 2024 08:48
@lukaszstolarczuk lukaszstolarczuk requested a review from lplewa April 11, 2024 09:43
@bratpiorka bratpiorka force-pushed the rrudnick_dis_l0_example branch from 926678b to 474fdc4 Compare April 11, 2024 10:45
@lukaszstolarczuk lukaszstolarczuk merged commit 943f674 into oneapi-src:main Apr 11, 2024
69 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vinser52 vinser52 vinser52 left review comments

@lplewa lplewa lplewa approved these changes

@lukaszstolarczuk lukaszstolarczuk lukaszstolarczuk approved these changes

@PatKamin PatKamin PatKamin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@bratpiorka @vinser52 @lplewa @lukaszstolarczuk @PatKamin