10000 BUG: Restore support for i386 and PowerPC (OS X) by evanmiller · Pull Request #20422 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

BUG: Restore support for i386 and PowerPC (OS X) #20422

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Nov 27, 2021
Merged

BUG: Restore support for i386 and PowerPC (OS X) #20422

merged 1 commit into from
Nov 27, 2021

Conversation

evanmiller
Copy link
Contributor

@charris
Copy link
Member
8000 charris commented Nov 21, 2021

That's i686, correct?

@charris
Copy link
Member
charris commented Nov 21, 2021

Also, long double on i686 (32 bit system) is usually 12 bytes for alignment of 80 bit extended precision.

@evanmiller
Copy link
Contributor Author

I believe it's i686; OS X refers to all 32-bit Intel as "i386" so I am going off that.

I will update the logic to reflect 12-byte alignment.

@mattip
Copy link
Member
mattip commented Nov 23, 2021

This is in response to #20270 which redefined NPY_SIZEOF_LONGDOUBLE and NPY_SIZEOF_COMPLEX_LONGDOUBLE for __x86_64 and __arm64__. Would it make sense only do the redefines if we are on one of those platforms, which would the leave in place the defines that worked before #20270 ?

@evanmiller
Copy link
Contributor Author

@mattip If I understand you correctly, you propose:

  • Moving the #undef statements to be inside the #if blocks
  • Removing the "unknown architecture" #error

Is that correct?

@mattip
Copy link
Member
mattip commented Nov 23, 2021

I think so. I am a bit undecided since it would be nice if all the definitions of NPY_SIZEOF_LONGDOUBLE and NPY_SIZEOF_COMPLEX_LONGDOUBLE were generated the way they are for the other platforms (via generate_numpyconfig_h()), but that might be out of scope for this PR. Maybe a comment "move to generate_numpyconfig_h in setup.py` might provide a hint for someone to redo this at some point. I am not sure why they are here in the first place, maybe because of cross-compilation?

@evanmiller
Copy link
Contributor Author

Maybe best to ask @isuruf

@isuruf
Copy link
Contributor
isuruf commented Nov 24, 2021

For universal2 wheels, we need the headers to be common to both architecture before preprocessing. That's why this was added.
There are 2 options here.

  1. Remove undef for arch other than x86_64, arm64. (This would support only universal2 wheels and thin wheels)
  2. Merge this PR. (This would support universal wheels in addition to universal2 and thin wheels)

I'm fine with either option. @evanmiller, thanks for fixing this.

@mattip
Copy link
Member
mattip commented Nov 24, 2021

Thanks, that is helpful. I am not very familiar with the nuances here, are universal wheels still supported across the ecosystem? @evanmiller whichever choice you make, could you add a comment summarizing the point @isuruf makes?

@evanmiller
Copy link
Contributor Author

@mattip Is the existing comment sufficient?

numpy/numpy/core/include/numpy/numpyconfig.h

Lines 6 to 10 in e94bbb8

/*
* On Mac OS X, because there is only one configuration stage for all the archs
* in universal builds, any macro which depends on the arch needs to be
* hardcoded
*/

@isuruf
Copy link
Contributor
isuruf commented Nov 26, 2021

Sounds good @evanmiller

@isuruf
Copy link
Contributor
isuruf commented Nov 26, 2021 
 /* 
  * On Mac OS X, because there is only one generated header for all the archs 
  * in universal builds, any macro in the generated header which depends on
  * the arch needs to be hardcoded.
  */

@mattip mattip merged commit 657e6a5 into numpy:main Nov 27, 2021
@mattip
Copy link
Member
mattip commented Nov 27, 2021

Thanks @evanmiller

@evanmiller evanmiller deleted the patch-1 branch November 27, 2021 13:29
@charris charris added the 09 - Backport-Candidate PRs tagged should be backported label Nov 27, 2021
@charris charris added this to the 1.22.0 release milestone Nov 27, 2021
@charris charris mentioned this pull request Nov 27, 2021
Merged
@charris charris added 06 - Regression and removed 09 - Backport-Candidate PRs tagged should be backported labels Nov 27, 2021
@charris charris removed this from the 1.22.0 release milestone Nov 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
00 - Bug 06 - Regression
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@evanmiller @charris @mattip @isuruf
0