Add float benchmark, and some minor float fixes/cleanup #2864
No reviewers
Labels
No labels
bazel
bluetooth
board settings
boot_stage2
bug
build
c++
can't reproduce
clang
clib-interface
cmsis
code cleanliness
code size
compiler version issues
cyw43_driver
debug
discussion
documentation
duplicate
dynamic_clock
elf2uf2
enhancement
errata
filesystem
flash layout
floating point
FreeRTOS
hardware_adc
hardware_claim
hardware_clocks
hardware_dma
hardware_exception
hardware_flash
hardware_gpio
hardware_i2c
hardware_irq
hardware_pio
hardware_pwm
hardware_rcp
hardware_rtc
hardware_spi
hardware_sync
hardware_uart
hardware_vreg
hardware_watchdog
hardware_xosc
help wanted
host
IAR
installation
invalid
linker scripts
lto
lwip
mbedtls
missing API
needs reporter detail
networking
newlib
newlib-multicore
non-c
non-cmake
non-gcc
openocd
orphan PR
OS integration
pico_async_context
pico_base
pico_binary_info
pico_flash
picolibc
pico_lwip
pico_malloc
pico_multicore
pico_printf
pico_runtime
pico_stdio
pico_stdio_usb
pico_sync
pico_time
pico_util
pioasm
port
question
review
review2
rtos interop
runtime init
static library
svd
test
tinyusb
vscode
wifi integration
wontfix
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
gh-raspberrypi/pico-sdk!2864
Loading…
Reference in a new issue
No description provided.
Delete branch "float-benchmark"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Actually converting to draft, as we should document the new defines (though they aren't really PICO_CONFIG as they are provided not options)
@ -198,0 +81,4 @@** On Arm, the following additional optimized functions are also provided when using `pico_double_pico`, all of which* saturate to the nearest representable value for too large input when converting from floating point types:*Huh? Line 54 says "On Arm, ..." and then this line says "Additional functions on Arm:" ??
@ -198,0 +129,4 @@#include "pico.h"// PICO_CONFIG: PICO_DOUBLE_IN_RAM, Force placement of SDK provided double-precision floating point code into RAM, type=bool, default=0, group=pico_doubleShould this be
group=pico_double? (defined on line 18 in this file)@ -0,0 +1,1342 @@#include <stdio.h>I guess the "dcmpge" name implies that this was supposed to be
?
@ -0,0 +309,4 @@return cycle_diff(t0, t1) - INT_INPUT_COST - DOUBLE_OUTPUT_COST;}timer_func_def(ui2d)(volatile int32_t i) {This might be a stupid question, but why does
timer_func_def(ui2d),timer_func_def(uint2double), etc. take avolatile int32_t iargument instead of avolatile uint32_t iargument? (Same question also for theuint64_tfunctions.)@ -0,0 +1,1342 @@#include <stdio.h>??
@ -0,0 +757,4 @@#endif}timer_func_def(fix642double_c)(volatile int64_t i) {This appears to be almost identical to
timer_func_def(fix642double)? (With I guess the only difference being that here thenparameter isconst)@ -0,0 +1,1286 @@#include <stdio.h>@ -198,0 +129,4 @@#include "pico.h"// PICO_CONFIG: PICO_DOUBLE_IN_RAM, Force placement of SDK provided double-precision floating point code into RAM, type=bool, default=0, group=pico_doubleIf we've got all these shiny new
PICO_DOUBLE_HAS_BLAH_CONVERSIONSdefines, maybe it makes sense to also use them incustom_double_funcs_test.c? (and similarly for float, of course)Bazel is complaining that it hasn't been told about
test/pico_float_test/double_benchmark.candtest/pico_float_test/float_benchmark.cyet 😉check-configs is complaining because there's no
#define PICO_DOUBLE_IN_RAM 0. In #2858 I added it tosrc/rp2_common/pico_double/double_v1_rom_shim_rp2040.S, copying what https://github.com/raspberrypi/pico-sdk/blob/develop/src/rp2_common/pico_float/float_v1_rom_shim_rp2040.S#L11 does.@ -0,0 +309,4 @@return cycle_diff(t0, t1) - INT_INPUT_COST - DOUBLE_OUTPUT_COST;}timer_func_def(ui2d)(volatile int32_t i) {because i didn't feel like adding even more variants for signed integers - the argument type is known so i just cast
@ -0,0 +757,4 @@#endif}timer_func_def(fix642double_c)(volatile int64_t i) {yup - we have optimized implementations for fixed exponents since there is a processor instruction
@ -0,0 +1,1286 @@#include <stdio.h>goot spot
@ -198,0 +129,4 @@#include "pico.h"// PICO_CONFIG: PICO_DOUBLE_IN_RAM, Force placement of SDK provided double-precision floating point code into RAM, type=bool, default=0, group=pico_doubleyup
@ -198,0 +81,4 @@** On Arm, the following additional optimized functions are also provided when using `pico_double_pico`, all of which* saturate to the nearest representable value for too large input when converting from floating point types:*oops
This is still the case 😉
@ -18,22 +18,23 @@#define test_checki64(x, expected, msg) ({ if ((x) != (expected)) { printf(" %s: %lld != %lld\n", msg, (int64_t)(x), (int64_t)(expected)); stop(); } })Maybe it makes sense to also use the
PICO_DOUBLE_HAS_macros here and on line 37?Lovely ❤️
@ -18,22 +18,23 @@#define test_checki64(x, expected, msg) ({ if ((x) != (expected)) { printf(" %s: %lld != %lld\n", msg, (int64_t)(x), (int64_t)(expected)); stop(); } })no the comment was meant to imply that is not the case, maybe it could be clearer
Yeah - i was fixing that but my change ended up ballooning
3 & 4 made things really messy after all, so I think I'll just stick with 1/2 for now
@ -198,0 +123,4 @@** mla/fma_fast** On RISC-V there is no custom double-precision floating point support, so `pico_double_pico` is equivalent to `pico_double_compiler`@ -135,75 +127,238 @@ extern "C" {*@ -135,75 +127,238 @@ extern "C" {*@ -198,0 +123,4 @@** mla/fma_fast** On RISC-V there is no custom double-precision floating point support, so `pico_double_pico` is equivalent to `pico_double_compiler`I wonder if these descriptions in
double.hshould say "...to the nearest double" ?@ -198,0 +123,4 @@** mla/fma_fast** On RISC-V there is no custom double-precision floating point support, so `pico_double_pico` is equivalent to `pico_double_compiler`?
@ -198,0 +123,4 @@** mla/fma_fast** On RISC-V there is no custom double-precision floating point support, so `pico_double_pico` is equivalent to `pico_double_compiler`@ -198,0 +123,4 @@** mla/fma_fast** On RISC-V there is no custom double-precision floating point support, so `pico_double_pico` is equivalent to `pico_double_compiler`@ -198,0 +123,4 @@** mla/fma_fast** On RISC-V there is no custom double-precision floating point support, so `pico_double_pico` is equivalent to `pico_double_compiler`I'm gonna assume yes, based on other descriptions in this header file!
@ -198,0 +123,4 @@** mla/fma_fast** On RISC-V there is no custom double-precision floating point support, so `pico_double_pico` is equivalent to `pico_double_compiler`@ -198,0 +123,4 @@** mla/fma_fast** On RISC-V there is no custom double-precision floating point support, so `pico_double_pico` is equivalent to `pico_double_compiler`@ -198,0 +123,4 @@** mla/fma_fast** On RISC-V there is no custom double-precision floating point support, so `pico_double_pico` is equivalent to `pico_double_compiler`Probably a stupid question, but is this needed? https://en.cppreference.com/w/c/header/float.html says that it just defines a bunch of constants starting with either
FLT_,DBL_orLDBL_; but I can't see any of those being used here?@ -198,0 +123,4 @@** mla/fma_fast** On RISC-V there is no custom double-precision floating point support, so `pico_double_pico` is equivalent to `pico_double_compiler`@ -198,0 +123,4 @@** mla/fma_fast** On RISC-V there is no custom double-precision floating point support, so `pico_double_pico` is equivalent to `pico_double_compiler`@ -198,0 +123,4 @@** mla/fma_fast** On RISC-V there is no custom double-precision floating point support, so `pico_double_pico` is equivalent to `pico_double_compiler`@ -198,0 +123,4 @@** mla/fma_fast** On RISC-V there is no custom double-precision floating point support, so `pico_double_pico` is equivalent to `pico_double_compiler`@ -198,0 +123,4 @@** mla/fma_fast** On RISC-V there is no custom double-precision floating point support, so `pico_double_pico` is equivalent to `pico_double_compiler`Does it matter that one of these is
double powint(double, int32_t)but the other isdouble powint(double, int)?@ -198,0 +123,4 @@** mla/fma_fast** On RISC-V there is no custom double-precision floating point support, so `pico_double_pico` is equivalent to `pico_double_compiler`I had to look up https://en.cppreference.com/w/c/numeric/math/fma to check the arguments here - is it worth adding a small comment to clarify that this function effectively returns
(x * y) + z?@ -135,75 +127,238 @@ extern "C" {*@ -135,75 +127,238 @@ extern "C" {*@ -135,75 +127,238 @@ extern "C" {*@ -442,10 +442,9 @@ int main() {for (double a = -100.0; a < 100.0; a += 53.103) {Is this right? The "FMA" and "FMAFAST" tests seem to be calling the same function?
@ -18,22 +18,23 @@#define test_checki64(x, expected, msg) ({ if ((x) != (expected)) { printf(" %s: %lld != %lld\n", msg, (int64_t)(x), (int64_t)(expected)); stop(); } })actually you were right
@ -442,10 +442,9 @@ int main() {for (double a = -100.0; a < 100.0; a += 53.103) {yes thankyou
@ -198,0 +123,4 @@** mla/fma_fast** On RISC-V there is no custom double-precision floating point support, so `pico_double_pico` is equivalent to `pico_double_compiler`it is a good question; i'm not sure why it was there but seems mean to remove it now in case someone is relying on it
@ -195,3 +1,1 @@#endif#endif/*@ -289,36 +445,53 @@ uint64_t float2ufix64(float f, int e);})@ -198,0 +123,4 @@** mla/fma_fast** On RISC-V there is no custom double-precision floating point support, so `pico_double_pico` is equivalent to `pico_double_compiler`Before this PR,
src/rp2_common/pico_float/include/pico/float.hdid#include <float.h>.This PR also adds
#include <float.h>tosrc/rp2_common/pico_double/include/pico/double.h, which is what I was (indirectly) querying.Yikes, CI builds are all failing with
@ -195,3 +1,1 @@#endif#endif/*@ -198,0 +205,4 @@#if !__PICO_DOUBLE_ARM_OPTIMIZED// for non Arm-optimized we may as well provide the function and let the compiler handle itstatic inline double int2double(int32_t i) { return (double)i; }static inline double uint2double(uint32_t i) { return (double)i; }GitHub seems to have had a brain-fart and added a load of unwanted blank lines all throughout some of the recent commits in this PR 😢
IMHO
((x * y) + z)would be slightly clearer here, but I guess it doesn't matter as long as you assume that it's using the "normal" operator-precedence rules 😉@ -195,3 +1,1 @@#endif#endif/*(to match the change in
float.h)@ -0,0 +223,4 @@#if PICO_FLOAT_HAS_SQRTF_FASTregister io_ro_32 *systick_ptr = systick_value_ptr();uint32_t t0 = *systick_ptr;volatile float x = sqrtf_fast(a);Is it intentional that this uses both
fsqrt_fastandsqrtf_fast, or should thetimer_func_defalso be usingsqrtf_fast?test/pico_float_test/pico_double_test.cis testingsqrt_fast- shouldtest/pico_float_test/pico_float_test.cbe testingsqrtf_fast?test/pico_float_test/pico_double_test.calso be testingddiv_fast?test/pico_float_test/pico_float_test.calso be testingfdiv_fast?P.S.
furious_fast(7)🏎️ 😆@ -195,3 +1,1 @@#endif#endif/*This should fix the build error 🤞
(undoing the whoopise that happened in
382b3e3535)@ -198,0 +123,4 @@** mla/fma_fast** On RISC-V there is no custom double-precision floating point support, so `pico_double_pico` is equivalent to `pico_double_compiler`yeah probably don't want that
@ -198,0 +205,4 @@#if !__PICO_DOUBLE_ARM_OPTIMIZED// for non Arm-optimized we may as well provide the function and let the compiler handle itstatic inline double int2double(int32_t i) { return (double)i; }static inline double uint2double(uint32_t i) { return (double)i; }grr
yeah i think that is sane for any programmer
@ -0,0 +223,4 @@#if PICO_FLOAT_HAS_SQRTF_FASTregister io_ro_32 *systick_ptr = systick_value_ptr();uint32_t t0 = *systick_ptr;volatile float x = sqrtf_fast(a);this is intentional
sure
already was
sure