lib/libm/wf_[lt]gamma: Define _IEEE_LIBM only if not set.

[agatti]

fdilibm was originally meant to see _IEEE_LIBM defined from outside the libm code, not it being hardcoded in (see section 4 in https://netlib.org/fdlibm/readme). Picolibc assumes this assumption holds true and attempts to define it itself, conflicting with the existing definition.

Although Picolibc provides its own libm support, on RISC-V the version of Picolibc installed on the CI machine currently crashes when running the math tests. This is a stop-gap solution that would make things work until either a fix for the test crashes is found or a newer version of Picolibc is provided by the CI machine's package repository.

Edit: Picolibc crashes due to its lgammaf implementation requiring TLS support enabled.

[github-actions]

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32:    +0 +0.000% PYBV10
     mimxrt:    +0 +0.000% TEENSY40
        rp2:    +0 +0.000% RPI_PICO_W
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.39%. Comparing base (ace08c3) to head (4978959).

❗ Current head 4978959 differs from pull request most recent head d7aa2fe

Please upload reports for the commit d7aa2fe to get more accurate results.

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #15183   +/-   ##
=======================================
  Coverage   98.39%   98.39%           
=======================================
  Files         161      161           
  Lines       21204    21204           
=======================================
  Hits        20864    20864           
  Misses        340      340           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[dpgeorge]

Since the code only ever uses #ifdef _IEEE_LIBM, this change is a no-op, so it's fine.

[dpgeorge]

Edit: Picolibc crashes due to its lgammaf implementation requiring TLS support enabled.

@agatti Given that knowledge, do you think the change in this PR is still a good way forward, does it fix the crash?

[agatti]

@dpgeorge I believe this is still needed. This PR will fix the case when you want to use Picolibc's libc with MicroPython's libm - the TLS issue is for when you want to use both Picolibc's libc and libm, and MicroPython for everything else.

I can add the TLS fix to #12853 if you think it's needed in the context of a RISC-V test environment to also work with an external libm implementation.

[agatti]

Better yet, with this PR in, the CI scripts can be updated to include Picolibc compatibility tests for Arm and Lx106 targets.

The CI machine can have picolibc-arm-none-eabi and picolibc-xtensa-lx106-elf packages installed, so the build scripts can be updated to make sure MicroPython also works with Picolibc for Arm and ESP8266 targets.

[projectgus]

The CI machine can have picolibc-arm-none-eabi and picolibc-xtensa-lx106-elf packages installed, so the build scripts can be updated to make sure MicroPython also works with Picolibc for Arm and ESP8266 targets.

Ooh, I think this would be very interesting! I've been side-eyeing the amount of newlib stuff that ends up in the stm32 builds lately, and wondering how much code size we'd save by using picolibc instead...

[projectgus]

the TLS issue is for when you want to use both Picolibc's libc and libm, and MicroPython for everything else.

For now is it reasonable for MicroPython to not support or test that combo, assuming the riscv picolibc builds work properly with MicroPython libm?

Similar to my previous comment, though, it might be interesting to experiment with picolibc libm at some point, as a comparison for code size, performance, and accuracy. I haven't looked, but I'm guessing it might be more optimised for riscv perhaps?

[projectgus]

(Off-topic comment about the fdlibm readme.)

Please send comments and bug reports to the electronic mail address
suggested by:
fdlibm-comments AT sun.com

This unexpectedly triggered a bit of nostalgia. 😿

[agatti]

the TLS issue is for when you want to use both Picolibc's libc and libm, and MicroPython for everything else.

For now is it reasonable for MicroPython to not support or test that combo, assuming the riscv picolibc builds work properly with MicroPython libm?

That's what I think as well. In fact if you get the latest qemu-riscv version from my work branch (https://github.com/agatti/micropython/tree/qemu-riscv/ - there's a wrong fixup in there but I didn't push a correction to not restart CI for nothing) and apply the libm modifications from this PR it should work.

Similar to my previous comment, though, it might be interesting to experiment with picolibc libm at some point, as a comparison for code size, performance, and accuracy. I haven't looked, but I'm guessing it might be more optimised for riscv perhaps?

As an aside, whilst waiting for something else to compile I did try that with minimal TLS initialisation code to see that for myself. Weirdly, the float/math_domain_special test fails as for Picolibc cosh(-INFINITY) equals -INFINITY instead of INFINITY. Haven't stepped through the binary to see what's going on, but Picolibc's code looks correct (it is the same as recent-ish newlib), so I may have missed something myself.

[dpgeorge]

I've been side-eyeing the amount of newlib stuff that ends up in the stm32 builds lately, and wondering how much code size we'd save by using picolibc instead...

That would be interesting to try, using picolibc instead of newlib.

In the meantime, this PR is good to merge.