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

Major cleanup. Remove compile, compile_wasm, and MixTape #146

Merged
merged 8 commits into from
Nov 11, 2023

Conversation

MasonProtter
Copy link
Collaborator

@MasonProtter MasonProtter commented Nov 10, 2023

This removes the compile functionality (closes #119), and does some general cleanup and fixes.

Before we merge this though, I'd also like to also discuss removing

  • the Mixtape stuff since I'm not really convinced it's worth the maintenance burden when we already have @device_override which is easier to use, and doesn't rely on the broken, unmaintained CodeInfoTools.jl. @device_override should cover every reasonable usecase as far as I'm aware.
  • The wasm stuff because https://github.com/tshort/WebAssemblyCompiler.jl exists and appears to work, whereas I'm not sure the wasm stuff in here works or not. Alternative we could look at bringing the functionality from that package in to here. cc @tshort

@MasonProtter
Copy link
Collaborator Author

Failing integration tests appear to be because of wasm.

@tshort
Copy link
Owner

tshort commented Nov 11, 2023

Thumbs up on this PR. I looked over the code, but I didn't really do a code review. If it passes tests, I'm good.

I'm okay with removing both the Mixtape and the wasm support. For wasm, I think WebAssemblyCompiler will (should) compile more code and offer more capability. I agree that the @device_override is way easier than Mixtape. For code I'm using (mainly WebAssemblyCompiler), I've used the overlays a lot but haven't needed Mixtape.

@tshort
Copy link
Owner

tshort commented Nov 11, 2023

As part of the cutting of features, I would like to keep the cross-compiling support.

Copy link
Collaborator

@brenhinkeller brenhinkeller left a comment

Choose a reason for hiding this comment

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

This looks good to me! This package has really come a long way and taught us all a lot I think. This slimming-down should be great for the maintenance burden going forward, and anyone can always check out the older versions if they want to see all the other things we experimented with.

I'm happy with merging whenever it seems like tests are in as good shape as we can get them, as long as Tom's happy too

@MasonProtter
Copy link
Collaborator Author

Okay, I've removed the wasm stuff, and the mixtape stuff. I also added an ability to specify your own method_table during compilation, in case there are cases where someone wants to do a certain overload just for a certain function they want to compile, but don't want to pollute the StaticCompiler wider method-table.

@MasonProtter MasonProtter changed the title Remove compile, do some cleaning up Major cleanup. Remove compile, compile_wasm, and MixTape Nov 11, 2023
@MasonProtter MasonProtter merged commit 3f9f236 into master Nov 11, 2023
28 of 32 checks passed
@brenhinkeller
Copy link
Collaborator

Hooray!

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.

remove compile?
3 participants