core/llvm_intrinsics: remove llvm.roundeven call from call_float_roundeven (issue 396) #428

abdul124 · 2024-06-20T12:21:52+08:00

abdul124 commented

2024-06-20 12:21:52 +08:00

The previous implementation uses llvm.roundeven which is not supported on all platforms as discussed in #396. Therefore, I have updated the call_float_roundeven function to make use of other LLVM intrinsic calls to provide functionality of llvm.roundeven.

The previous implementation uses `llvm.roundeven` which is not supported on all platforms as discussed in #396. Therefore, I have updated the `call_float_roundeven` function to make use of other LLVM intrinsic calls to provide functionality of `llvm.roundeven`.

sb10q commented

2024-06-20 12:49:56 +08:00

Hmm, are there any other codebases doing this?

abdul124 commented

2024-06-20 13:20:33 +08:00

Hmm, are there any other codebases doing this?

I did not find a similar issue in other codebases online. However, by using simpler llvm.floor calls instead of llvm.roundeven, the new function should work on all platforms supporting LLVM floor operations.

> Hmm, are there any other codebases doing this? I did not find a similar issue in other codebases online. However, by using simpler `llvm.floor` calls instead of `llvm.roundeven`, the new function should work on all platforms supporting LLVM floor operations.

sb10q commented

2024-06-20 13:26:10 +08:00

Well OK but will it return the same/correct results?

abdul124 commented

2024-06-20 13:34:14 +08:00

Well OK but will it return the same/correct results?

It passed the testcases in nac3standalone/demo/ndarray.py file. The functionality was relatively simple (round to nearest integer using even numbers to break tie), so it should return the same results as llvm.roundeven.

> Well OK but will it return the same/correct results? It passed the testcases in nac3standalone/demo/ndarray.py file. The functionality was relatively simple (round to nearest integer using even numbers to break tie), so it should return the same results as llvm.roundeven.

sb10q commented

2024-06-20 13:34:59 +08:00

Floating point stuff can be deceptive. Please look at a reference roundeven implementation e.g. from glibc.

abdul124 commented

2024-06-20 15:54:07 +08:00

Floating point stuff can be deceptive. Please look at a reference roundeven implementation e.g. from glibc.

I compared the implementation with the implementation of floor and roundeven in glibc. The operations in glibc perform bitwise manipulation on the integer to implement the rounding logic. Looking at the sequence of operations in the floor and roundeven functions in the glibc library, the current implementation of call_float_roundeven is missing a check for NaN case. Should I add that as well? Also, are floating values in our case always positive or can they be negative as well?

> Floating point stuff can be deceptive. Please look at a reference roundeven implementation e.g. from glibc. I compared the implementation with the implementation of floor and roundeven in glibc. The operations in glibc perform bitwise manipulation on the integer to implement the rounding logic. Looking at the sequence of operations in the floor and roundeven functions in the glibc library, the current implementation of call_float_roundeven is missing a check for NaN case. Should I add that as well? Also, are floating values in our case always positive or can they be negative as well?

sb10q commented

2024-06-20 15:56:48 +08:00

I'm not sure if doing another manual implementation of this function is the right thing to do in the first place.
Indeed there are often edge cases with NaN, denormal numbers, etc.

I'm not sure if doing another manual implementation of this function is the right thing to do in the first place. Indeed there are often edge cases with NaN, denormal numbers, etc.

abdul124 commented

2024-06-20 17:34:26 +08:00

I'm not sure if doing another manual implementation of this function is the right thing to do in the first place.

As an alternative, I can replace the llvm.roundeven with llvm.rint or llvm.round in the code. However, their exact functionality is slightly different from roundeven at tie cases. I still believe that it would be more straightforward to use a combination of LLVM intrinsic functions to implement the rounding logic instead.

Indeed there are often edge cases with NaN, denormal numbers, etc.

I am using llvm.floor and llvm.frem for rounding to avoid handling of all these cases myself. Also, I have added the quick return cases from glibc implementation (return at NaN or Integer). The new structure is similar to the one used in:
https://github.com/lattera/glibc/blob/master/sysdeps/ieee754/dbl-64/s_roundeven.c
I have also added some edge cases to the math.py file to test the functionality

> I'm not sure if doing another manual implementation of this function is the right thing to do in the first place. As an alternative, I can replace the `llvm.roundeven` with `llvm.rint` or `llvm.round` in the code. However, their exact functionality is slightly different from roundeven at tie cases. I still believe that it would be more straightforward to use a combination of LLVM intrinsic functions to implement the rounding logic instead. > Indeed there are often edge cases with NaN, denormal numbers, etc. I am using `llvm.floor` and `llvm.frem` for rounding to avoid handling of all these cases myself. Also, I have added the quick return cases from glibc implementation (return at NaN or Integer). The new structure is similar to the one used in: https://github.com/lattera/glibc/blob/master/sysdeps/ieee754/dbl-64/s_roundeven.c I have also added some edge cases to the math.py file to test the functionality

sb10q commented

2024-06-20 17:36:33 +08:00

The main requirement is the correctness of the numpy function that currently uses roundeven, i.e. it must always return exactly the same result as numpy, including on edge cases.

Reimplementing roundeven in LLVM IR is quite awkward and error prone, as I hinted in previous comments.

The main requirement is the correctness of the numpy function that currently uses roundeven, i.e. it must always return exactly the same result as numpy, including on edge cases. Reimplementing roundeven in LLVM IR is quite awkward and error prone, as I hinted in previous comments.

abdul124 force-pushed issue-396 from c7c7da7ca2 to 14d95ff5a5

2024-06-24 15:02:39 +08:00

Compare

abdul124 force-pushed issue-396 from 14d95ff5a5 to 1835561217

2024-06-26 13:34:57 +08:00

Compare

abdul124 commented

2024-06-26 14:05:38 +08:00

Using llvm.roundeven was breaking since musl libc did not provide an implementation of the roundeven function. Instead of adding a roundeven function patch for platforms with musl libc, I made use of llvm.nearbyint (https://llvm.org/docs/LangRef.html#llvm-nearbyint-intrinsic). musl libc provides an implementation for nearbyint, and therefore the resulting function works for both glibc and musl libc platforms.
I discovered a similar solution being employed here: https://support.xilinx.com/s/question/0D52E00006hpQ42SAE/roundevenf-undefined-reference-hls?language=en_US

Using `llvm.roundeven` was breaking since `musl libc` did not provide an implementation of the roundeven function. Instead of adding a roundeven function patch for platforms with `musl libc`, I made use of `llvm.nearbyint` (https://llvm.org/docs/LangRef.html#llvm-nearbyint-intrinsic). `musl libc` provides an implementation for `nearbyint`, and therefore the resulting function works for both `glibc` and `musl libc` platforms. I discovered a similar solution being employed here: https://support.xilinx.com/s/question/0D52E00006hpQ42SAE/roundevenf-undefined-reference-hls?language=en_US

sb10q commented

2024-06-26 14:08:27 +08:00

musl libc provides an implementation for nearbyint

But the core device does not, please add it to the runtime (artiq and artiq-zynq).

Why was this not required with the legacy compiler?

> musl libc provides an implementation for nearbyint But the core device does not, please add it to the runtime (artiq and artiq-zynq). Why was this not required with the legacy compiler?

sb10q merged commit 83154ef8e1 into master

2024-07-03 14:17:52 +08:00

sb10q deleted branch issue-396

2024-07-03 14:17:53 +08:00

Sign in to join this conversation.

No reviewers