Skip to content

RTL: fix an issue with abs() #1835

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

Merged
merged 1 commit into from
May 18, 2023
Merged

RTL: fix an issue with abs() #1835

merged 1 commit into from
May 18, 2023

Conversation

certik
Copy link
Contributor

@certik certik commented May 18, 2023

Previously the abs() was somehow returning a negative number, because we called it with an int8_t argument, but it was expecting "int" argument.

The other bug was that "num" in the for loop stayed at -1 forever. We fixed that by using the condition "num > 0". We also switched to a while loop which make it more readable.

Fixes #1825.

Previously the abs() was somehow returning a negative number, because we
called it with an int8_t argument, but it was expecting "int" argument.

The other bug was that "num" in the for loop stayed at -1 forever. We
fixed that by using the condition "num > 0". We also switched to a while
loop which make it more readable.
@certik certik requested a review from Smit-create May 18, 2023 17:05
Copy link
Collaborator

@Smit-create Smit-create left a comment

Choose a reason for hiding this comment

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

Thanks, It's clean!

@Smit-create Smit-create merged commit d731c7a into lcompilers:main May 18, 2023
@Shaikh-Ubaid
Copy link
Collaborator

Team, does this actually fix #1825?

On latest main, compiling using the following commands

./build0.sh
cmake -DWITH_FMT=yes -DCMAKE_CXX_FLAGS_RELEASE="-Wall -Wextra -O3 -funroll-loops -DNDEBUG" -DWITH_LLVM=yes .
cmake --build . -j16

I am experiencing the following:

$ python integration_tests/test_bit_length.py               
7
3
7
7
$ lpython integration_tests/test_bit_length.py               
AssertionError

@Shaikh-Ubaid
Copy link
Collaborator

Shaikh-Ubaid commented May 18, 2023

In #1835 (comment), am I using the correct/expected flags for release mode compilation?

Also,

$ cat integration_tests/test_bit_length.py 
from math import floor, log2
from lpython import i8, i32, i16

def ff3():
    x: i16
    one: i16
    one = i16(1)
    x = -i16(one << i16(13))
    print("ff3(): x.bit_length() =", x.bit_length(), ", where x is", x)
    assert i32(x.bit_length()) == 14

ff3()
$ python integration_tests/test_bit_length.py 
ff3(): x.bit_length() = 14 , where x is -8192
$ lpython integration_tests/test_bit_length.py
ff3(): x.bit_length() = 0 , where x is -8192
AssertionError

@certik
Copy link
Contributor Author

certik commented May 18, 2023

@Shaikh-Ubaid can you open up a new issue? It looks like you are getting a failure:

$ lpython integration_tests/test_bit_length.py               
AssertionError

But it works on my laptop as well as the CI. Let's try to reproduce the problem that you are seeing and then we'll fix it.

@Shaikh-Ubaid
Copy link
Collaborator

@Shaikh-Ubaid can you open up a new issue? It looks like you are getting a failure:But it works on my laptop as well as the CI. Let's try to reproduce the problem that you are seeing and then we'll fix it.

Sure. Opened here #1836.

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.

_lpython_bit_length1 fails using conda-forge lpython
3 participants