Skip to content

Implemented multi-device sample - #91

Merged
bashbaug merged 2 commits into
KhronosGroup:mainfrom
StreamHPC:multi-device-sample
Dec 14, 2023
Merged

Implemented multi-device sample#91
bashbaug merged 2 commits into
KhronosGroup:mainfrom
StreamHPC:multi-device-sample

Conversation

@Beanavil

@Beanavil Beanavil commented Oct 27, 2023

Copy link
Copy Markdown
Member

This PR adds the C/C++ samples showcasing a multi-device convolution computation that makes use of sub-devices and sub-buffers.

Note: only the last commit is relevant for the PR, all the other commits are from the StreamHPC:release-cd branch. After #88 is merged, this should be trivial to rebase/merge.

@CLAassistant

CLAassistant commented Oct 27, 2023

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@Beanavil
Beanavil force-pushed the multi-device-sample branch 4 times, most recently from 87a4154 to 419fe40 Compare October 30, 2023 11:09

@bashbaug bashbaug left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Note: I only reviewed the last commit in this PR.

Comment thread samples/core/multi-device/main.c Outdated
Comment thread samples/core/multi-device/main.c Outdated
Comment thread samples/core/multi-device/main.c Outdated
Comment thread samples/core/multi-device/main.c Outdated
Comment thread samples/core/multi-device/main.c Outdated
Comment thread samples/core/multi-device/main.c Outdated
Comment thread samples/core/multi-device/main.c Outdated
Comment thread samples/core/multi-device/main.cpp Outdated
Comment thread samples/core/multi-device/main.cpp Outdated
Comment thread samples/core/multi-device/main.cpp Outdated

@bashbaug bashbaug left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Getting closer but a few issues remain, and there are a few new ones.

Comment thread samples/core/multi-device/main.c Outdated
Comment thread samples/core/multi-device/main.c Outdated
@Beanavil
Beanavil force-pushed the multi-device-sample branch from b818605 to b33c0e4 Compare December 7, 2023 14:20

@bashbaug bashbaug left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Latest version LGTM, at least the last two commits (the ones for the multi-device sample specifically). Thanks!

You may want to keep just those two commits in this PR because the other infrastructure-related commits are still under review.

@Beanavil
Beanavil force-pushed the multi-device-sample branch from b33c0e4 to 0babaf4 Compare December 12, 2023 09:08
@Beanavil

Copy link
Copy Markdown
Member Author

Rebased branch and removed unrelated commits. So I think this is ready to be merged :)

@bashbaug
bashbaug requested a review from nikhiljnv December 12, 2023 17:28
@bashbaug

Copy link
Copy Markdown
Contributor

Requested a review from @nikhiljnv, planning to merge tomorrow.

@nikhiljnv nikhiljnv left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM except a minor comment to fix the limit for cl_device_partition_property.

Comment thread samples/core/multi-device/main.c Outdated
@Beanavil
Beanavil force-pushed the multi-device-sample branch from 0babaf4 to b6475e4 Compare December 13, 2023 12:06

@nikhiljnv nikhiljnv left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Approved.

@Beanavil
Beanavil requested a review from bashbaug December 13, 2023 12:12
@bashbaug

Copy link
Copy Markdown
Contributor

Merging as agreed in the December 12th teleconference.

@bashbaug
bashbaug merged commit b818f59 into KhronosGroup:main Dec 14, 2023
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.

5 participants