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

Unsigned ints no wrap #2176

Merged
merged 7 commits into from
Jul 19, 2023
Merged

Conversation

Shaikh-Ubaid
Copy link
Collaborator

@Shaikh-Ubaid Shaikh-Ubaid commented Jul 18, 2023

towards #2173

@Shaikh-Ubaid
Copy link
Collaborator Author

This is a Work in progress (WIP). The tests might not pass.

@Shaikh-Ubaid
Copy link
Collaborator Author

Shaikh-Ubaid commented Jul 18, 2023

$ cat examples/expr2.py    
from lpython import u16, bitnot_u16, i32

u: u16 = u16(255)
u = bitnot_u16(u)
print(u)
print(~i32(u))
$ lpython examples/expr2.py
65280
255
$ python examples/expr2.py 
65280
-65281

It seems we are unable to use bitnot() and ~ successively.

@certik
Copy link
Contributor

certik commented Jul 18, 2023

The bitnot seems correct but ~ doesn't work? We have to debug it.

@Shaikh-Ubaid
Copy link
Collaborator Author

That issue is fixed. The following is a concern:

$ cat examples/expr2.py 
from lpython import i16, u64, bitnot_u64

w: u64 = u64(123457889998923)
x: u64 = bitnot_u64(w)
print(w, i16(w), ~i16(w))
print(x, i16(x), ~i16(x))
$ python examples/expr2.py 
123457889998923 123457889998923 -123457889998924
18446620615819552692 18446620615819552692 -18446620615819552693
$ lpython examples/expr2.py
123457889998923 -26549 26548
18446620615819552692 26548 -26549

It seems u8, u16, u32 and u64 need to be some functions that could convert values appropriately in CPython.

@certik
Copy link
Contributor

certik commented Jul 18, 2023

the i16 and u16 in LPython must give a runtime (or compile time) error when the value you are converting is out of range. The contract is that if LPython works, then CPython must work. So in CPython we don't need to do any checking and all these casts become a no-op.

@Shaikh-Ubaid
Copy link
Collaborator Author

If the casts in CPython become no-op, the output value of CPython is not matching the output of LPython (Example: #2176 (comment)).

@Shaikh-Ubaid
Copy link
Collaborator Author

For unsigned unary minus, shall we throw error in LPython similar to unsigned bit not?

@certik
Copy link
Contributor

certik commented Jul 18, 2023

The contract is that if LPython compiles and runs this without errors, then CPython produces the same value. The design of all i8-i64, u8-u64 is such that they are just no-op in CPython and they always produce exactly the same number as in LPython, since LPython only allows such casts that don't change the value. If the value is out of range, we give an error message. So this design should work.

@Shaikh-Ubaid
Copy link
Collaborator Author

only allows such casts that don't change the value. If the value is out of range, we give an error message.

Got it. Thank you so much for the clarification!

@certik
Copy link
Contributor

certik commented Jul 18, 2023

For unsigned unary minus, shall we throw error in LPython similar to unsigned bit not?

Given that -0 = 0, the unary minus should be allowed for unsigned, but obviously it will only work for 0, for all other values it must give a runtime error.

@Shaikh-Ubaid
Copy link
Collaborator Author

Shaikh-Ubaid commented Jul 19, 2023

The CI might temporarily pass now. As a next step, we need to support

  • unsigned unary minus (based on Unsigned ints no wrap #2176 (comment))
  • uncomment the specific tests for unary minus (and possibly update if the values are not in appropriate range)
  • uncomment the specific tests for bitshift and update the tests so that it does not overflow.
  • add error tests for ~ and -.

@Shaikh-Ubaid
Copy link
Collaborator Author

Further steps could be to support compile time overflow checking and later runtime error checking for which we have a dedicated issue here #2183.

@certik
Copy link
Contributor

certik commented Jul 19, 2023

Otherwise it looks good. After you fix the above, please do not merge it, I have to manually check and update our internal codes if needed before we can merge it.

Copy link
Contributor

@certik certik left a comment

Choose a reason for hiding this comment

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

I think this change is good. I'll mark it as draft, so that we don't accidentally merge it. I need to see if anything broke first.

@certik certik marked this pull request as draft July 19, 2023 13:53
@certik
Copy link
Contributor

certik commented Jul 19, 2023

I tested the compiled mode (LPython) and it gives a nice error message (it could be improved to give back exactly the expression to use, but it's good enough for now) and after using bitnot_u64 it works.

I now need to test the emulation mode (lpython.py), which is the more fragile mode that might have some bugs.

@certik certik marked this pull request as ready for review July 19, 2023 15:33
@certik certik merged commit 4339407 into lcompilers:main Jul 19, 2023
@certik
Copy link
Contributor

certik commented Jul 19, 2023

Everything works for us, so I am merging this PR.

Thanks @Shaikh-Ubaid !

@certik
Copy link
Contributor

certik commented Jul 19, 2023

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.

2 participants