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

adds a program build interface to compile for a specific device #102

Merged
merged 2 commits into from
Sep 1, 2020

Conversation

bashbaug
Copy link
Contributor

@bashbaug bashbaug commented Jun 9, 2020

Fixes #60.

This complements the existing program build interface to compile for a vector of devices.

@kpet
Copy link
Contributor

kpet commented Jun 10, 2020

Should we add a test?

bashbaug added 2 commits June 10, 2020 12:03
This complements the existing program build interface to compile for a vector of devices.
@bashbaug bashbaug force-pushed the 60-program-specialization branch from ed2737d to f1bbd85 Compare June 10, 2020 22:00
@bashbaug
Copy link
Contributor Author

Should we add a test?

I'm pretty sure the answer to this question is almost always "yes" 😃.

I've added a test for the new interface with a single device since it was relatively straightforward. I was looking to add a test for the existing interface that accepts a vector of devices also, but this turned out to be deceptively difficult, so this PR only tests the new interface.

The main issue is that we've defined a special buildErrHandler for the program build and compile interfaces (but not the program link interface, interestingly enough). This error handler gets used instead of the standard errHandler. The buildErrHandler unconditionally queries the program build log for all devices and includes it in a special BuildError exception if an error occurs. Because the build log is queried unconditionally, it requires a lot more to be mocked to test this interface.

Is this functionality useful, or should the build interfaces be handled via the standard errHandler instead? Note that the sample in the C++ bindings header will work even if we do this, since it queries the build log as part of the exception handler, vs. obtaining it from the exception itself:

        cl::Program vectorAddProgram(programStrings);
        try {
            vectorAddProgram.build("-cl-std=CL2.0");
        }
        catch (...) {
            // Print build info for all devices
            cl_int buildErr = CL_SUCCESS;
            auto buildInfo = vectorAddProgram.getBuildInfo<CL_PROGRAM_BUILD_LOG>(&buildErr);
            for (auto &pair : buildInfo) {
                std::cerr << pair.second << std::endl << std::endl;
            }

            return 1;
        }

@kpet
Copy link
Contributor

kpet commented Jun 12, 2020

I'm pretty sure the answer to this question is almost always "yes" smiley.

This was more of a rhetorical question. Seeing an approved PR adding new functionality without a test, I felt compelled :).

I think buildErrHandler and the unconditional gathering of build logs is useful functionality. Most users will want to see the build log after a failure. Having it directly packaged as part of the exception makes sense to me. Otherwise you need to catch build exceptions separately and then query the build log (or have a special case in the exception handler), this means more code for a very common pattern. Besides, performance really isn't a concern here.

Now I understand how that is a bit of a pain for testing. Maybe we could have utility testing functions to mirror what buildErrHandler and the typical calling code do?

Copy link
Contributor

@kpet kpet left a comment

Choose a reason for hiding this comment

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

@bashbaug To be clear: I'm fine with this PR as is. Improving testing for existing functionality needn't be done under this PR.

@bashbaug
Copy link
Contributor Author

I think we should merge this PR as-is. I would like to look at a utility testing function that mirrors buildErrHandler, but I don't expect to have a chance to look at this in the near future.

@kpet
Copy link
Contributor

kpet commented Sep 1, 2020

Created #112 so we don't forget to add the utility function. Merging as agreed on the 2020/09/01 teleconference.

@kpet kpet merged commit fee9d2d into KhronosGroup:master Sep 1, 2020
@kpet kpet mentioned this pull request Sep 2, 2020
@bashbaug bashbaug deleted the 60-program-specialization branch June 5, 2023 23:16
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.

Please add specialization cl::Program::build(cl::Device&)
3 participants