Skip to content
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

BUG: New meson system fails to identify long double representation on 64-bit BE platforms #23972

Closed
matoro opened this issue Jun 17, 2023 · 7 comments · Fixed by #23982
Closed
Labels
00 - Bug Meson Items related to the introduction of Meson as the new build system for NumPy

Comments

@matoro
Copy link
Contributor

matoro commented Jun 17, 2023

Describe the issue:

When using the new Meson build system, numpy fails to configure on 64-bit big-endian platforms:

Checking for size of "long double" : 16 (cached)

../numpy-1.25.0/numpy/core/meson.build:379:4: ERROR: Problem encountered: No idea what this is ....

Seems to be due to this new snippet:

# long double representation detection (see setup_common.py)
# TODO: this is still incomplete, and different from how it's done in the
# numpy.distutils based build, see https://github.com/mesonbuild/meson/issues/11068
longdouble_size = cc.sizeof('long double')
if longdouble_size == 8
  if host_machine.endian() == 'little'
    longdouble_format = 'IEEE_DOUBLE_LE'
  else
    longdouble_format = 'IEEE_DOUBLE_BE'
  endif
elif longdouble_size == 12
  error('This should not be possible, 12 bits of "content" should still result in sizeof() being 16. Please report this error!'
  )
elif longdouble_size == 16
  if host_machine.endian() == 'little'
    # FIXME: this varies, there's multiple formats here! Not yet implemented.
    #        TBD how we deal with the mess of old long double formats.
    longdouble_format = 'INTEL_EXTENDED_16_BYTES_LE'
  else
    error('No idea what this is ....')
  endif
else
  error('Unknown long double size: ' + londouble_size)
endif
cdata.set10('HAVE_LDOUBLE_' + longdouble_format, true)

This affects such platforms as: ppc64 BE, mips64 BE, s390x, sparc64. 32-bit BE platforms such as hppa, ppc, m68k are unaffected.

Downstream bug: https://bugs.gentoo.org/908738

Reproduce the code example:

N/A

Error message:

No response

Runtime information:

N/A

Context for the issue:

No response

@rgommers
Copy link
Member

That's indeed the relevant TODO. xref mesonbuild/meson#11068. It's all fixable, just needs someone to spend time on it.

@matoro why is this a Gentoo bug against 1.25.0? You should still be building that with numpy.distutils rather than meson.

@matoro
Copy link
Contributor Author

matoro commented Jun 18, 2023

That's indeed the relevant TODO. xref mesonbuild/meson#11068. It's all fixable, just needs someone to spend time on it.

@matoro why is this a Gentoo bug against 1.25.0? You should still be building that with numpy.distutils rather than meson.

I specified the version range in the Gentoo bug because that's the version the downstream maintainer decided to go ahead and switch over to meson, so it is relevant to the downstream maintainer (but not to upstream).

Sam has gone ahead and dropped keywords anyway in gentoo/gentoo@f096a78 so we will be individually revalidating each architecture.

@rgommers
Copy link
Member

Thanks @matoro, that's good to know. The actual in-memory format detection needs work, but it's easy to hardcode the most common 64-bit big endian format now, that will probably fix most of those issues. The options are:

  • IBM_DOUBLE_DOUBLE_BE
  • IEEE_QUAD_BE
  • BINFMT_NAME IEEE_binary128_be
  • MOTOROLA_EXTENDED_12_BYTES_BE

To try, you could change the line error('No idea what this is ....') in the snippet in the issue description to longdouble_format = <above-string-here>.

@rgommers rgommers added the Meson Items related to the introduction of Meson as the new build system for NumPy label Jun 18, 2023
@matoro
Copy link
Contributor Author

matoro commented Jun 18, 2023

Thanks @matoro, that's good to know. The actual in-memory format detection needs work, but it's easy to hardcode the most common 64-bit big endian format now, that will probably fix most of those issues. The options are:

* `IBM_DOUBLE_DOUBLE_BE`

* `IEEE_QUAD_BE`

* `BINFMT_NAME IEEE_binary128_be`

* `MOTOROLA_EXTENDED_12_BYTES_BE`

To try, you could change the line error('No idea what this is ....') in the snippet in the issue description to longdouble_format = <above-string-here>.

That might get it to compile, but I don't think it will be fully accurate until the entire original logic was ported. In fact, I don't think even the LE logic is correct right now. For example, standard IEEE long doubles are not being introduced in ppc64le on Fedora until Fedora 38. So this would compile incorrect code on Fedora 37 ppc64le right now.

I will look into mesonbuild/meson#11068 and see if I can port over the logic.

@thesamesam
Copy link
Contributor

Thanks matoro and ralf!

@matoro why is this a Gentoo bug against 1.25.0? You should still be building that with numpy.distutils rather than meson.

We're pretty keen to get onto Meson as soon as we can for numpy because it's so much faster to build without -j1 (especially on slower platforms) and the support for cross, but we're also very happy to then help with any bugs that fall out of that. Sorry for the confusion!

If needed, we can add a warning to the packaging to ask users to definitely not report any issues upstream for now without asking us first to avoid confusion.

In this case, I'd meant to drop the tags marking it as working on everything-but-amd64 but didn't until matoro noticed - so now it goes through the usual testing process.

@eli-schwartz
Copy link

Fantastic work, nice to see that this was ultimately practical to do entirely via the compiler object methods in meson.

@rgommers
Copy link
Member

If needed, we can add a warning to the packaging to ask users to definitely not report any issues upstream for now without asking us first to avoid confusion.

I think it's fine to open bug reports, we're aiming to complete the switchover before the well before the Python 3.12 release date (Oct 4th), so whatever gets reported is probably something we need to address anyway.

One word of warning: most SIMD acceleration isn't enabled in the 1.25.x branch (nor in main yet), so while the build is a lot faster, Gentoo users may see a performance impact as a result of that missing SIMD support.

charris pushed a commit to charris/numpy that referenced this issue Jun 24, 2023
This ports the old Python code for identifying the long double
representation to C, so that it can be easily invoked by meson.  The
original implementation is at https://github.com/numpy/numpy/blob/eead09a3d02c09374942cdc787c0b5e4fe9e7472/numpy/core/setup_common.py#L264-L434

The C portion of the code has been tested and confirmed to work on
systems with the following formats, either natively or via an
alternative ABI:  INTEL_EXTENDED_16_BYTES_LE, IEEE_QUAD_BE,
IEEE_QUAD_LE, IBM_DOUBLE_DOUBLE_BE, IBM_DOUBLE_DOUBLE_LE,
IEEE_DOUBLE_BE, INTEL_EXTENDED_12_BYTES_LE.

The original meson port includes an error condition with the comment
"This should not be possible, 12 bits of "content" should still result
in sizeof() being 16."  As far as I can tell this is incorrect, as
compiling on an x86_64 system with 32-bit ABI (gcc -m32) does indeed
have sizeof(long double)==12.  This is reflected in the C code.

Closes numpygh-23972, closes
mesonbuild/meson#11068.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
00 - Bug Meson Items related to the introduction of Meson as the new build system for NumPy
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants