MAINT: LoongArch: switch away from the __loongarch64 preprocessor macro #28092
Conversation
| @@ -668,7 +668,7 @@ npy__cpu_init_features(void) | |||
|
|
|||
| /***************** LoongArch ******************/ | |||
|
|
|||
| #elif defined(__loongarch64) | |||
| #elif defined(__loongarch__) | |||
There was a problem hiding this comment.
We don't have an environment for loongarch32, so we only did validation on loongarch64. So, to be cautious, I recommend using the __loongarch_lp64 macro here. After the loongarch32 environment is validated in the future, we will replace it with __loongarch__ macro.
There was a problem hiding this comment.
I know. This is specifically for avoiding to have to make another change and wait for another release, in the likely case that the final ratified LA32 ABI requires no modification here.
Look: eventually this line is going to say __loongarch__, so if we do so right now, a future PR adding LA32 support would have one less line of change. As long as LA32's basic C and ELF ABI (sizeof(int), availability of libc functionality, AT_HWCAP definitions, this kind of thing) remains within expectation, we should be safe here.
There was a problem hiding this comment.
I think if we support LA32 in the future, we can revise it uniformly. It is recommended to use LA64 now better.
There was a problem hiding this comment.
I think if we support LA32 in the future, we can revise it uniformly. It is recommended to use LA64 now better.
I don't agree with your points, because to me it's overly conservative about future. Are you not confident enough the API is sensible enough to not change?
But if you insist, I'll change for one time to not block progress. Let's add some diffstat to the future LA32 patchset then... ¯\_(ツ)_/¯
|
Is this ready? |
| @@ -668,7 +668,7 @@ npy__cpu_init_features(void) | |||
|
|
|||
| /***************** LoongArch ******************/ | |||
|
|
|||
| #elif defined(__loongarch64) | |||
| #elif defined(__loongarch__) | |||
There was a problem hiding this comment.
| #elif defined(__loongarch__) | |
| #elif defined(__loongarch_lp64) |
No, one suggestion has not yet been revised. |
According to the *Toolchain Conventions of the LoongArch Architecture* specification (v1.1) [1], the `__loongarch64` preprocessor macro is in fact a legacy symbol retained for compatibility; the reason being that the GPR width and the calling convention in use are orthogonal to each other, and in reality we care about the calling convention (e.g. ILP32 or LP64) more often than about the hardware capability -- think of ILP32 code on 64-bit hardware, in which case the GPR is wider than what a pointer actually can occupy, for example. As for the two modified places, the header change is actually concerned with the calling convention; and while the implementation change is actually correct regardless of bitness or calling convention, the Loongson maintainer prefers staying with `__loongarch_lp64` to be extra safe against possible API changes for the future ILP32 ABI. [1]: https://github.com/loongson/la-toolchain-conventions/blob/releases/v1.1/LoongArch-toolchain-conventions-EN.adoc
I expressly don't agree with your point, but in order to not block progress, I've rebased with your suggestion applied. Fortunately the ILP32 enablement work is already underway so we'll be able to revisit this in short order. |
LGTM. Now, it's safer to do that. In future patches of LA32, we wiil discuss it. |
There was a problem hiding this comment.
LGTM, let's get this in since the two experts have agreed (or agreed to disagree). Either option would have been fine with us I think. In it goes, thanks @xen0n and @ErnstPeng!
According to the Toolchain Conventions of the LoongArch Architecture specification (v1.1) 1, the
__loongarch64preprocessor macro is in fact a legacy symbol retained for compatibility; the reason being that the GPR width and the calling convention in use are orthogonal to each other, and in reality we care about the calling convention (e.g. ILP32 or LP64) more often than about the hardware capability -- think of ILP32 code on 64-bit hardware, in which case the GPR is wider than what a pointer actually can occupy, for example.As for the two modified places, the header change is actually concerned with the calling convention, and the implementation change is actually correct regardless of bitness or calling convention. So, use
__loongarch_lp64for the header, and just__loongarch__for the implementation.cc @ErnstPeng -- this polishes up the preprocessor usage of #27856.