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

Remove OpenGLCompute #8077

Merged
merged 6 commits into from
Feb 11, 2024
Merged

Remove OpenGLCompute #8077

merged 6 commits into from
Feb 11, 2024

Conversation

steven-johnson
Copy link
Contributor

This was supposed to be removed in Halide 17 (oops), removing for Halide 18

This was supposed to be removed in Halide 17 (oops), removing for Halide 18
@steven-johnson steven-johnson added the release_notes For changes that may warrant a note in README for official releases. label Feb 7, 2024
@steven-johnson steven-johnson marked this pull request as ready for review February 7, 2024 23:36
Copy link
Contributor

@shoaibkamil shoaibkamil left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for doing this!

@TH3CHARLie Do we need to bump the serialization version? I'm not sure what our approach has been when deprecating features (maybe this is the first time since serialization was added).

@steven-johnson
Copy link
Contributor Author

LGTM, thanks for doing this!

@TH3CHARLie Do we need to bump the serialization version? I'm not sure what our approach has been when deprecating features (maybe this is the first time since serialization was added).

IMHO we should probably bump the version for every Halide release, regardless

@TH3CHARLie
Copy link
Contributor

LGTM, thanks for doing this!

@TH3CHARLie Do we need to bump the serialization version? I'm not sure what our approach has been when deprecating features (maybe this is the first time since serialization was added).

I think anything that changes the serialization definition needs to bump the serialization version, on the minor number(i.e. 0.1.0 -> 0.1.1). And second to Steven's comment, we should bump the serialization version number on every major release.

@steven-johnson
Copy link
Contributor Author

LGTM, thanks for doing this!
@TH3CHARLie Do we need to bump the serialization version? I'm not sure what our approach has been when deprecating features (maybe this is the first time since serialization was added).

I think anything that changes the serialization definition needs to bump the serialization version, on the minor number(i.e. 0.1.0 -> 0.1.1). And second to Steven's comment, we should bump the serialization version number on every major release.

#8080

@steven-johnson steven-johnson merged commit 22581bf into main Feb 11, 2024
19 checks passed
@steven-johnson steven-johnson deleted the srj/oglc branch February 11, 2024 18:40
@TH3CHARLie
Copy link
Contributor

One thing I found about this PR is that, the serialization version numbers help users to find out they are deserializing from an old binary pipeline. But on the other hand, if you are using an old Halide build with a updated binary pipeline, the code may crash before we start to check the version numbers (because of a format change in this PR in particular):

const auto *pipeline_obj = Serialize::GetPipeline(data.data());
(GetPipeline would crash internally here I believe)

ardier pushed a commit to ardier/Halide-mutation that referenced this pull request Mar 3, 2024
* Remove OpenGLCompute

This was supposed to be removed in Halide 17 (oops), removing for Halide 18

* Update dynamic_allocation_in_gpu_kernel.cpp

* Update dynamic_allocation_in_gpu_kernel.cpp

* Update halide_ir.fbs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_notes For changes that may warrant a note in README for official releases.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants