MAINT: Make npy_half a strong type #21487
Conversation
b68c046 to
f804432
Compare
There was a problem hiding this comment.
Some of numpy_half manipulating functions are promoted to inline to avoid
performance regression.
This is worrying me. Is the calling convention for this struct different from the uint16? Because npymath is a library, so the symbols must remain ABI compatible.
Further, the npy_half definition is public. I think we can only modify it if explicitly enabled by the user. I would suggest creating a #define NPY_HALF_IS_STRONG definition (which we always use internally) and documenting it, then use an #ifdef.
The main concern right now is the ABI compatibility within npy_math though.
|
Hmmmpf, I can't see way to work around any possible ABI issues. I think we need to keep the Might it work to use macro "aliases" to expand calls to the original (possibly now inlined) functions and add the |
|
Can you elaborate on why you think this is an ABI breakage? |
|
It definitely isn't on x64. But is a small struct guaranteed to be passed by value in the same register as the equivalent small integer type? |
|
Ouch, I hadn't considered that the behavior could be platform specific. I guess that's the type of thing that |
f804432 to
62868e4
Compare
|
I 100% agree, this would not break ABI compatibility on X86_64 linux machines, but would break on other archs / and on Windows that passes all struct argument on the stack. I'll submit a patch that maintain ABI compatibility soon. |
4670308 to
e0d9b39
Compare
|
@seberg please tell me if you're fine with current approach: it keeps old symbols while not making them available in the header file, so that each recompiled file will use the new interface. However, the old symbols are still compiled in in the |
| extern int npy_half_signbit(npy_half h); | ||
| extern npy_half npy_half_neg(npy_half h); | ||
| extern npy_half npy_half_abs(npy_half h); | ||
| extern npy_half npy_half_pos(npy_half h); |
There was a problem hiding this 10000 comment.
All of these seem fine to move into the header. Maybe just for the sake of it: can we add an (since NumPy 1.24, 2022-05) to the comment so in 5-10 years someone might decide to delete it without too much worry?
We want them as part of the ABI "for backwards compatibility".
There was a problem hiding this comment.
Comment updated, but we don't want these declarations in the header, this leads to multiple definitions. We want them once in this compilation unit.
e0d9b39 to
073d38e
Compare
|
The error seems unrelated, otherwise good for review :-) |
There was a problem hiding this comment.
Not sure if I like the NPY_HALF_INTERNAL_API macro/name (mainly, it doesn't really seem to safe any characters compared to just using the new name?).
And, users will actually be using the "internal" one now...
Since users will be switched over, this is a C-API break. Now the problem I see is, that I am unsure how you would write code that will compile fine against both an old and a new NumPy version. Not being able to do that would be inconvenient.
So, I still think it might be good to introduce an "opt-in" to using the struct version? And probably keep the default at not using it (except for NumPy itself). Or am I missing an easy transition here?
numpy/core/include/numpy/halffloat.h
Outdated
| #define NPY_HALF_NEGONE (npy_half){0xbc00u} | ||
| #define NPY_HALF_PINF (npy_half){0x7c00u} | ||
| #define NPY_HALF_NINF (npy_half){0xfc00u} | ||
| #define NPY_HALF_NAN (npy_half){0x7e00u} |
There was a problem hiding this comment.
Not sure about binding, but may need extra backets to write NPY_HALF_ZERO.bits?
Yeah, it's a C-API break. In most situation, a recompilation should be just fine, but in some cases adding a few cast /
In that case, maybe we should just have two headers, |
|
I am not worried about the API break itself. Users can keep up and there are not many half users. But such a user should have an easy way to adapt their code so that it will comile against both old and new NumPy versions (and that should be explained in a release note). Two headers may be a good solution, I am not sure right now where |
ok, and to achieve that a macro-controlled API should be just fine. I understand you want that macro to activate old API by default? If we do so what would be the incentive for client code to move to new API? |
|
Honestly, I didn't think about it enough. I am also OK to default to the new definition (modulo any pushback that comes up). |
796b9dd to
75a5855
Compare
|
@seberg: if you agree with this version, I'll had a release note and a test case for the legacy API |
|
I think I am happy with this. Just to summarize for others, the proposal is to:
Macro magic will do the rest ;). |
|
Sounds reasonable |
75a5855 to
a8ea24f
Compare
|
Update done, with the appropriate test case (that detected a few errors in the legacy ABI ;-)) |
a8ea24f to
cc85dcb
Compare
|
@serge-sans-paille you add a "fragment" file into |
Instead of using a type alias, make npy_half a struct. As a consequence, cleanup npy_half usage to always reference function declared in numpy/halffloat.h. This avoids vodoo incantation in files that should now nothing of npy_half internal. Some of numpy_half manipulating functions are promoted to inline to avoid performance regression, but symbols are still generated. This is an ABI breakage. In order to keep ABI compatibility, older symbols are still built, and new symbols are prefixed by `npy_strongly_typed_half_` instead of `npy_half_`. This change should be invisible to the end-user as soon as they include numpy/halffloat.h. If for any reason, new applications still need to be compiled against the legacy API, NPY_USE_LEGACY_HALF must be defined prior to including numpy/halffloat.h. This is not used by any numpy internal code. A new test, numpy/core/src/npymath/_half_legacy_tests.c, tests that all legacy symbols are till available.
cc85dcb to
d76db1b
Compare
|
@seberg : done! |
|
Thanks, I don't feel like pushing this in before branching, but bug me if I don't follow-up/review this soon after branching happened :). |
|
@seberg is now the good time? :-) |
|
It would be, but I am now a bit confused about the fact that |
|
As valiant as this PR was. @seiko2plus do you think your half PR largely supersedes this? I realize that this (by default) actually changed the public |
IMHO, defining half-precision as struct rather than an alias of |
|
OK, lets close it then for now. I guess there might still be some interesting tidbits here, but there is also the moving part that we probably want to get rid of |
Instead of using a type alias, make npy_half a struct.
As a consequence, cleanup npy_half usage to always reference function declared in
numpy/halffloat.h. This avoids vodoo incantation in files that should now
nothing of npy_half internal.
Some of numpy_half manipulating functions are promoted to inline to avoid
performance regression.