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

[GPU/OpenCL] Handling pre-compiled and existing kernels #2535

Merged
merged 2 commits into from
Apr 18, 2024

Conversation

s-debadri
Copy link
Contributor

Handling existing kernels:

  • Modified clCreateKernel to read pre-compiled kernel binaries.
  • Handling already initialized kernels using global bitmask.
  • Added enumerator for kernel names and a method to resolve as string.

Signed-off-by: Debadri Samaddar s.debadri@samsung.com

Added feature for reading kernel binaries.
Managing already created kernels.
Added static flag and bitmask to check existing kernels.

Signed-off-by: Debadri Samaddar <s.debadri@samsung.com>
@taos-ci
Copy link
Collaborator

taos-ci commented Apr 4, 2024

📝 TAOS-CI Version: 1.5.20200925. Thank you for submitting PR #2535. Please a submit 1commit/1PR (one commit per one PR) policy to get comments quickly from reviewers. Your PR must pass all verificiation processes of cibot before starting a review process from reviewers. If you are new member to join this project, please read manuals in documentation folder and wiki page. In order to monitor a progress status of your PR in more detail, visit http://ci.nnstreamer.ai/.

Copy link
Collaborator

@taos-ci taos-ci left a comment

Choose a reason for hiding this comment

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

@s-debadri, 💯 All CI checkers are successfully verified. Thanks.

@myungjoo
Copy link
Member

myungjoo commented Apr 5, 2024

LGTM.

Later, kernel path policy should be aware of security and privileges of Android and Tizen devices. In such devices, it should be able to accept different paths per app or the caller.

However, as long as the kernel path is configurable, it can wait until opencl support becomes mature.

Copy link
Collaborator

@dkjung dkjung left a comment

Choose a reason for hiding this comment

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

LGTM


// checking bitmask for already initialized kernel. eg: 010 & 000 -> 0 but 010
// & 110 -> 010
if (layerKernel == (kernelInitializedMask & layerKernel)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

if (layerKernel & kernelInitializedMask) {

I think that this statement has the same effect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes...nice observation.

getKernelName(layerKernel) + "_kernel.bin",
std::ios::binary | std::ios::in);

if (fs) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (fs) {
if (fs.good()) {

Is there any case that fs can be null?

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 don't think the fs object can be null.

Updated ifstream object valid condition

Signed-off-by: Debadri Samaddar <s.debadri@samsung.com>
Copy link
Collaborator

@taos-ci taos-ci left a comment

Choose a reason for hiding this comment

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

@s-debadri, 💯 All CI checkers are successfully verified. Thanks.

Copy link
Collaborator

@jijoongmoon jijoongmoon left a comment

Choose a reason for hiding this comment

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

LGTM

@jijoongmoon jijoongmoon merged commit 8581435 into nnstreamer:main Apr 18, 2024
29 checks passed
@s-debadri s-debadri deleted the create_kernel_utility branch May 23, 2024 07:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants