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

Add fast single-precision add/sub/mul for Hazard3 #1883

Merged
merged 9 commits into from
Aug 30, 2024

Conversation

Wren6991
Copy link
Contributor

Some people have noted the libgcc soft float implementations are quite slow, so:

  • Add fast implementations for single-precision add/sub/mul (replacing __addsf3, __subsf3 and __mulsf3 symbols)
  • Use pico float implementation by default on RISC-V
  • Remove macro definitions for h3.bextm/h3.bextmi from pico_asm_helper (as they clash with the correct versions in hardware/hazard3.h)
  • Add some tests for the new float functions (the Arm tests didn't build at all, I've left them as-is)

Performance (including the function return but excluding the call):

  • __addsf3 is 46 cycles for the common case of adding two normals with normal result
    • (a few cycles extra for an edge case where rounding up increases the exponent; all other cases are faster)
  • __subsf3 is one cycle more than __addsf3
  • __mulsf3 is 35 cycles worst case

IEEE compliance:

  • Subnormal inputs and outputs are flushed to zero
  • NaN significands propagate correctly (signs of NaN outputs are don't-care)
  • Follows IEEE rules for NaN and infinity propagation and generation
  • Rounding mode is fixed at round to nearest, even on tie (and rounds correctly within this mode)
  • Does not set any global floating point exception flags (but generates quiet NaNs in exceptional cases)

I think this is a reasonable compromise for this SDK (noting the default libgcc implementation doesn't propagate NaN significands correctly).

@Wren6991 Wren6991 requested a review from kilograham August 29, 2024 12:09
Copy link
Contributor

@kilograham kilograham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also suggest for more clarity, updating float.h to not define any of the functions on RISC-V since they aren't implemented (still with COMBINED_DOCS guard)

You can still include hazard3.h to get everything. This just allows you
to pull in less.
* SPDX-License-Identifier: BSD-3-Clause
*/

#ifndef _HARDWARE_HAZARD3_FEATURES_
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note SDK convention is _FOO_H - this seems to have snuck thru in the original hazard3.h - I thought @lurch had a check for that somewhere.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i can fix

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought @lurch had a check for that somewhere.

Yeah, that's just for the board-header checking though ( tools/check_board_header.py ). I guess I could pull that out into a separate script which checks all .h files...

@kilograham kilograham merged commit d886df6 into develop Aug 30, 2024
2 checks passed
@kilograham kilograham deleted the hazard3-single-float branch August 30, 2024 16:36
@kilograham kilograham added this to the 2.0.1 milestone Aug 30, 2024
@lurch
Copy link
Contributor

lurch commented Sep 1, 2024

This PR actually seems to have broken the compilation of the SDK for RISC-V?

$ cd /tmp
$ wget https://buildbot.embecosm.com/job/corev-gcc-ubuntu2204/47/artifact/corev-openhw-gcc-ubuntu2204-20240530.tar.gz
$ tar xf corev-openhw-gcc-ubuntu2204-20240530.tar.gz
$ git clone --depth 1 --branch develop https://github.com/raspberrypi/pico-sdk
$ cd pico-sdk
$ mkdir build
$ cd build
$ cmake .. -DPICO_TOOLCHAIN_PATH=/tmp/corev-openhw-gcc-ubuntu2204-20240530 -DPICO_PLATFORM=rp2350-riscv
$ make
[  0%] Building ASM object src/rp2350/boot_stage2/CMakeFiles/bs2_default.dir/compile_time_choice.S.o
In file included from /tmp/pico-sdk/src/rp2350/boot_stage2/boot2_w25q080.S:29,
                 from /tmp/pico-sdk/src/rp2350/boot_stage2/compile_time_choice.S:18:
/tmp/pico-sdk/src/rp2350/pico_platform/include/pico/asm_helper.S:11:10: fatal error: hardware/hazard3/instructions.h: No such file or directory
   11 | #include "hardware/hazard3/instructions.h"
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
make[2]: *** [src/rp2350/boot_stage2/CMakeFiles/bs2_default.dir/build.make:75: src/rp2350/boot_stage2/CMakeFiles/bs2_default.dir/compile_time_choice.S.o] Error 1
make[1]: *** [CMakeFiles/Makefile2:2095: src/rp2350/boot_stage2/CMakeFiles/bs2_default.dir/all] Error 2
make: *** [Makefile:91: all] Error 2

Also, https://github.com/raspberrypi/pico-sdk/blob/develop/src/rp2_common/hardware_hazard3/include/hardware/hazard3.h seems to have a

#ifdef __cplusplus
extern "C" {
#endif

but no matching

#ifdef __cplusplus
}
#endif

?

To prevent similar errors in future, maybe it's worth adding RISC-V compilation to the GitHub CI? 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants