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

Various suggestions for changes and improvements. #35

Merged
merged 17 commits into from
Jan 28, 2023
Merged

Conversation

Ulrik-dk
Copy link
Contributor

  • Renames SequentialC to C, and MulticoreC to Multicore, in line with Futharks convention. This also simplifies the code in a number of places.
  • Unifies the _gen_c functions by using to_feature() instead of having different functions.
  • Removes the "None" backend.
  • Simplifies the code in a number of places, and handles some clippy warnings.
  • Panics and stops the process if the futhark compilation fails, instead of potentially failing silently.
  • Adds optimization flags for the different backends in build.rs.
  • Begins an implementation of explicit free(), in order to allow handling of Missing deallocation of context object #15. Implementing Drop would require a much more involved rewrite it seems. Being able to explicitly call free() instead might be good enough, and work.

@Erk-
Copy link
Owner

Erk- commented Nov 17, 2022

Looks like some good changes.

  • Begins an implementation of explicit free(), in order to allow handling of Missing deallocation of context object #15. Implementing Drop would require a much more involved rewrite it seems. Being able to explicitly call free() instead might be good enough, and work.

Regarding this point, I think the free function should be marked unsafe as the caller have to ensure that no methods to it are ever used again else we would run into a use-after-free.

The solution would likely be to remove the inner arc from the type and resolve the issue in that way, though this leads to some degradation of the user experience so maybe the api has to be changed around a bit for it.


Another way would be to save a flag if it has been freed and then panic, if that is done we don't need unsafe.

@Erk- Erk- self-requested a review November 17, 2022 12:31
@Ulrik-dk Ulrik-dk marked this pull request as ready for review December 22, 2022 22:46
src/genc.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/entry.rs Outdated Show resolved Hide resolved
src/genc.rs Outdated Show resolved Hide resolved
src/static/static_context.rs Show resolved Hide resolved
@Ulrik-dk Ulrik-dk force-pushed the master branch 2 times, most recently from 2b1dfd3 to 3ffdea9 Compare January 28, 2023 14:16
Ulrik-dk and others added 17 commits January 28, 2023 15:16
* Remove "None" backend, as this has no practical value. Just call futhark with `c` as the default backend.
* Made genfut's string versions of the backends agree with futharks. sequential_c -> c, multicore_c -> multicore. This simplifies the code. Yes it's less user friendly - but that's a problem with futhark. Let's inherit an imperfect convention instead of competing with it.
* Unified all the superfluous generator-functions that were identical, except they each used a different string constant for the backend.
* Updating futhark package dependencies should be up to the user, not only because it is slow, but because one may want to use something other than the newest version for various reasons.
* It also created an empty lib folder whenever it was called, which is very ugly.
@Ulrik-dk
Copy link
Contributor Author

I used the built-in github "sync fork" functionality, but that turns out to merge the master branch into the fork, so I undid it, and found a way to rebase instead. This is why the history has been messed up a bit.

@Ulrik-dk Ulrik-dk requested review from Erk- and Munksgaard and removed request for Erk- and Munksgaard January 28, 2023 14:18
@Ulrik-dk Ulrik-dk requested review from Munksgaard and Erk- and removed request for Munksgaard January 28, 2023 14:18
Copy link
Owner

@Erk- Erk- left a comment

Choose a reason for hiding this comment

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

Thanks for the pr! Will merge it now and touch up some things, then aim to have a pr out by the end of the weekend.

@Erk- Erk- merged commit 51c4614 into Erk-:master Jan 28, 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.

3 participants