-
-
Notifications
You must be signed in to change notification settings - Fork 10.9k
MAINT: remove duplicated integer code #8856
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
Conversation
this removes about 200KiB (~20%) code of the loops.o |
Looks pretty good to me. This doesn't help with my changeset in #8853 though, which includes some function names with integer typecodes ( What do you propose I do instead? Should we adjust |
To be clear, what you have doesn't appear to break anything I want to do, but it doesn't go quite far enough to be used in those places too. |
NPY_FUNCNAME needs to be used in all places, shouldn't be an issue. Let's merge #8853 first. |
I'm not sure this is worth doing, as it complicates the code with no real gain except some smallering. |
This has been discussed before many years ago... My first choice if we go this way would be to eliminate |
I think the goal is good, because we've seen multiple bugs due to code expecting longlong and getting int64 or similar. I haven't looked at the code yet though :-) |
Oh, I see, this only affects some deep internals, not the typecodes API. No strong opinion then. |
I'm a definite +1 on this if we can adjust how typecodes are resolved to function names with |
Our historical integer names contain a duplicate for example long and longlong are int64 on 64 bit platforms or int and long are int32 on 32 bit platforms. This causes quite significant code bloat, in particular for functions compiled for multiple instruction sets. This duplication can't be removed as it is part of our API but we can avoid some of the bloat by only generating code once for each real type and correctly mapping them to the right slot in the ufuncs type list. This is achieved by defining appropriate macros for the current platform and creating the correct function names via macro concatenation.
82e9611
to
2f7c97c
Compare
Would this work on platforms that don't have a 64-bit integer type? Wouldn't we end up referring to a type like |
I don't think the new code is too bad and the duplication and the increased compilation time does bother me. Though an issue is that system macros can get in the way of the function name expansion, e.g. copysign is a macro on windows. This avoided by adding the underscore in the first NPY_FUNCNAME expansion but it remains a theoretical possibility. This doesn't change the typecodes, while I'd love to change them I don't think we can, they are baked into our API. I don't think we support systems without 64 bit integers at the compiler level, if we do that configuration likely hasn't been tested in 20 years. |
There's an alarming line then in
Note that C99 defines |
that code exists specifically to handle the issue that we don't know what size the types have, so we check them at compile time in a large ifdef. The exact same approach is used in this PR. It would also work for longlong = char should such a system exist (spoiler, it doesn't). |
I'm not proposing we change those - just change the mapping between typecodes and the function names used in
Except it wouldn't in the context of More importantly, this doesn't define a |
My compiles are pretty quick as is. The question is simplicity and maintenance, especially for new folks who aren't familiar with the code. Note that compile times on my system are significantly faster if I delete previous builds. That is weird... |
ah yes you are correct there, still it shouldn't really matter in practice. The situation would also be easily fixable with an ifdef. |
I think suddenly not having any loops for |
We have to continue to accept all old typecodes so long as we maintain ABI compatibility, but we could normalize them internally and on output. We could even make the typecodes numerically identical so new source builds always use the normalized codes regardless of how they spell them. |
Where does this stand in light of current and ongoing work. |
@eric-wieser Still interested in this? |
This looks reasonable to me. We could also use the integer types in |
I am going to close this. At some point we could remove the redundant int types across the board, and this PR could point the way but I think the conflicts indicate the code has changed significantly since this was proposed. If someone disagrees, they can reopen or resubmit the changes as a new PR and champion getting it updated and reviewed. |
Our historical integer names contain a duplicate for example long and
longlong are int64 on 64 bit platforms or int and long are int32 on 32
bit platforms.
This causes quite significant code bloat, in particular for functions
compiled for multiple instruction sets.
This duplication can't be removed as it is part of our API but we can
avoid some of the bloat by only generating code for once the each real
type and correctly mapping them to the right slot in the ufuncs type
list.
This is achieved by defining appropriate macros for the current platform
and creating the correct function names via macro concatenation.