jameysharp requested cfallin for a review on PR #4989.
jameysharp opened PR #4989 from cleanup-frontend
to main
:
This PR eliminates most of the per-block state tracked by the
frontend
module in cranelift-frontend. I think I can eliminate the last bits of state while preserving all the debug-assertions, but I want to think about that more.I tried to keep the individual commits well-separated so it may be worth reviewing them individually.
This saves a tiny bit of memory allocations and traffic, according to DHAT, but has basically no impact on performance.
abrown submitted PR review.
bjorn3 created PR review comment:
Saltwater uses this function. (and so does it fork crabwater that aims to revive saltwater).
bjorn3 submitted PR review.
cfallin submitted PR review.
jameysharp submitted PR review.
jameysharp created PR review comment:
I gather you mean https://github.com/nulla-git/crabwater/? I didn't know that existed.
bjorn3 submitted PR review.
bjorn3 created PR review comment:
Yes
jameysharp submitted PR review.
jameysharp created PR review comment:
After discussion with Chris, I'm going to merge this despite that breakage. For anyone who's using this API, I'd be happy to help with migrating to something else after a discussion about what they actually need from it.
jameysharp merged PR #4989.
cfallin submitted PR review.
cfallin created PR review comment:
More specifically, to give some reasoning for this:
- We don't guarantee stability of Cranelift APIs between releases (each release is a semver break, and we haven't declared any sort of long-term stability guarantee generally);
- Thus, it's technically allowed to alter APIs arbitrarily;
- All the same, we want to minimize churn, as it's definitely in the project's interest to bring users along to new versions with minimal work. So we should have a reason to remove or alter APIs;
- In this case, @jameysharp 's reasoning (with which I think I agree) is that the function just seems a bit redundant or unnecessary: it lets the user query whether they have already added instructions, but it seems better either for the user to know that directly, or (possibly) to get that via more general IR introspection.
So on balance, I think this API cleanup makes sense, but we also don't want to cause arbitrary and unfixable breakage so please do let us know (anyone) if there are compatibility concerns we can try to address some other way.
abrown submitted PR review.
abrown created PR review comment:
(also, is no one else chuckling about the puns in all these names?!)
Last updated: Dec 23 2024 at 12:05 UTC