Unsigned Integer Support #213

ychenfo · 2022-03-06T04:36:50+08:00

ychenfo commented

2022-03-06 04:36:50 +08:00

Add support for uint32 and uint64, with corresponding integer type conversions and irrt support.

One thing unsure about the u64 constant support:

Currently I attempted to change the parser output of constant integers from i64 to u64 instead of i128 or a (u64, sign). Performance-wise i64 would be better than using i128 or (u64, sign), but now the constant integer bound check will be not available, and the interpretation of types of constant integers will be entirely depending on the annotations like int64(..) or uint32(..) or no special annotation(which will be int32).

Should we use

the current way
or use i128 or (u64, sign) for a more comprehensive support for u64 constants
or do not support u64 constants directly since they seems to be less common? (they should still be able to be created by leveraging llvm constant optimization, for example to get u64::MAX:

a = uint64(1) << uint64(63)
print_uint64((a - uint64(1)) + a)

output ir under current optimization:

define void @__modinit__() local_unnamed_addr personality i32 (...)* @__nac3_personality {
init:
 tail call void @print_uint64(i64 -1)
 ret void
}

print:

print_uint64: 18446744073709551615

or some other ways?

Add support for `uint32` and `uint64`, with corresponding integer type conversions and irrt support. --- **One thing unsure about the `u64` constant support:** Currently I attempted to change the parser output of constant integers from `i64` to `u64` instead of `i128` or a `(u64, sign)`. Performance-wise `i64` would be better than using `i128` or `(u64, sign)`, but now the constant integer bound check will be not available, and the interpretation of types of constant integers will be entirely depending on the annotations like `int64(..)` or `uint32(..)` or no special annotation(which will be int32). Should we use - the current way - or use `i128` or `(u64, sign)` for a more comprehensive support for `u64` constants - or do not support `u64` constants directly since they seems to be less common? (they should still be able to be created by leveraging llvm constant optimization, for example to get `u64::MAX`: ```python a = uint64(1) << uint64(63) print_uint64((a - uint64(1)) + a) ``` output ir under current optimization: ```llvm define void @__modinit__() local_unnamed_addr personality i32 (...)* @__nac3_personality { init: tail call void @print_uint64(i64 -1) ret void } ``` print: ``` print_uint64: 18446744073709551615 ``` - or some other ways?

ychenfo force-pushed uint from 562054bca1 to ba7afc8928

2022-03-06 04:38:01 +08:00

Compare

ychenfo commented

2022-03-06 04:38:54 +08:00

force-pushed to rebase on the latest master branch

sb10q commented

2022-03-06 10:30:15 +08:00

What about making the parser represent:

positive numbers that fit in u64 as Int(Some(u64)).
positive numbers that do not fit in u64 as Int(None).
negative numbers whose absolute value fits in u64 as USub(Int(Some(u64))).
negative numbers whose absolute value does not fit in u64 as USub(Int(None)).

This way we do not lose any useful information at the parsing stage, and I think the code structure of the compiler should be the simplest with this design.

Note that the compiler could still report an overflow e.g. when processing USub(Int(Some(u64))), or when converting Int(Some(u64)) into i64.

What about making the parser represent: * positive numbers that fit in ``u64`` as ``Int(Some(u64))``. * positive numbers that do not fit in ``u64`` as ``Int(None)``. * negative numbers whose absolute value fits in ``u64`` as ``USub(Int(Some(u64)))``. * negative numbers whose absolute value does not fit in ``u64`` as ``USub(Int(None))``. This way we do not lose any useful information at the parsing stage, and I think the code structure of the compiler should be the simplest with this design. Note that the compiler could still report an overflow e.g. when processing ``USub(Int(Some(u64)))``, or when converting ``Int(Some(u64))`` into ``i64``.

ychenfo commented

2022-03-07 04:41:15 +08:00

What about making the parser represent:

positive numbers that fit in u64 as Int(Some(u64)).

positive numbers that do not fit in u64 as Int(None).

negative numbers whose absolute value fits in u64 as USub(Int(Some(u64))).

negative numbers whose absolute value does not fit in u64 as USub(Int(None)).

This way we do not lose any useful information at the parsing stage, and I think the code structure of the compiler should be the simplest with this design.

Note that the compiler could still report an overflow e.g. when processing USub(Int(Some(u64))), or when converting Int(Some(u64)) into i64.

Thanks for the suggestion! After thinking and trying it for some time I think this would be good, as long as we are careful to handle a deeper ast representing integer constants in type check and codegen when we are traversing the ast.

> What about making the parser represent: > * positive numbers that fit in ``u64`` as ``Int(Some(u64))``. > * positive numbers that do not fit in ``u64`` as ``Int(None)``. > * negative numbers whose absolute value fits in ``u64`` as ``USub(Int(Some(u64)))``. > * negative numbers whose absolute value does not fit in ``u64`` as ``USub(Int(None))``. > > This way we do not lose any useful information at the parsing stage, and I think the code structure of the compiler should be the simplest with this design. > > Note that the compiler could still report an overflow e.g. when processing ``USub(Int(Some(u64)))``, or when converting ``Int(Some(u64))`` into ``i64``. Thanks for the suggestion! After thinking and trying it for some time I think this would be good, as long as we are careful to handle a deeper ast representing integer constants in type check and codegen when we are traversing the ast.

ychenfo force-pushed uint from ba7afc8928 to e7df755435

2022-03-07 04:41:34 +08:00

Compare

ychenfo commented

2022-03-07 04:42:08 +08:00

force-pushed to rebase on the latest master branch

ychenfo commented

2022-03-07 05:02:55 +08:00

as long as we are careful to handle a deeper ast representing integer constants in type check and codegen when we are traversing the ast.

Update: deeper ast for constant should be fine without too much modification, we can modify the parser to limit the depth of ast representing int constants to be smaller than 2, and then handle these in the type check and codegen. I will finish the detailed implementation tomorrow.

> as long as we are careful to handle a deeper ast representing integer constants in type check and codegen when we are traversing the ast. Update: deeper ast for constant should be fine without too much modification, we can modify the parser to limit the depth of ast representing int constants to be smaller than 2, and then handle these in the type check and codegen. I will finish the detailed implementation tomorrow.

pca006132 commented

2022-03-07 11:53:25 +08:00

Why not just i128? I don't think the performance penalty would be that large: we basically never touch the integer until we turn them into constants, and we will figure out their type when we turn them into constants. The memory overhead is not large either, Option<u64> will likely occupy similar size due to alignment requirements in 64 bits machine.

I think i128 will be a lot simpler to handle without so many different cases.

Why not just i128? I don't think the performance penalty would be that large: we basically never touch the integer until we turn them into constants, and we will figure out their type when we turn them into constants. The memory overhead is not large either, `Option<u64>` will likely occupy similar size due to alignment requirements in 64 bits machine. I think i128 will be a lot simpler to handle without so many different cases.

sb10q commented

2022-03-07 11:55:40 +08:00

While they are uncommon, there would be a problem with constants (including in non-NAC3 code) that do not fit in i128.

pca006132 commented

2022-03-07 11:57:36 +08:00

Indeed, a simple hack would be to encode those constants with a i128 value that is invalid in nac3 code, such as i128::MAX. Option<i128> would require a bit larger size.

Indeed, a simple hack would be to encode those constants with a i128 value that is invalid in nac3 code, such as `i128::MAX`. `Option<i128>` would require a bit larger size.

pca006132 commented

2022-03-07 11:58:31 +08:00

Yes it should be fine.

sb10q commented

2022-03-07 12:04:05 +08:00

we can modify the parser to limit the depth of ast representing int constants to be smaller than 2, and then handle these in the type check and codegen

I didn't realize there would be an issue there. The i128::MAX option seems ok.

> we can modify the parser to limit the depth of ast representing int constants to be smaller than 2, and then handle these in the type check and codegen I didn't realize there would be an issue there. The ``i128::MAX`` option seems ok.

ychenfo commented

2022-03-08 00:27:08 +08:00

Thanks for the suggestions! I will revert the changes and use i128::MAX instead.

Thanks for the suggestions! I will revert the changes and use `i128::MAX` instead.

ychenfo force-pushed uint from e7df755435 to 50bc5ee361

2022-03-08 02:38:49 +08:00

Compare

sb10q reviewed 2022-03-08 08:40:56 +08:00

nac3core/src/typecheck/magic_methods.rs Outdated

						
				@ -316,6 +323,20 @@ pub fn set_primitives_magic_methods(store: &PrimitiveStore, unifier: &mut Unifie

				    impl_comparison(unifier, store, int64_t, int64_t);

				    impl_eq(unifier, store, int64_t);

				    for t in [uint32_t, uint64_t] {

sb10q commented

2022-03-08 08:40:56 +08:00

Why not t in [int32_t, int64_t, uint32_t, uint64_t] and remove the two blocks above?

Why not ``t in [int32_t, int64_t, uint32_t, uint64_t]`` and remove the two blocks above?

ychenfo commented

2022-03-08 13:17:36 +08:00

Oh ok I will merge the two blocks above into this loop

ychenfo commented

2022-03-08 13:44:44 +08:00

Done

ychenfo marked this conversation as resolved

ychenfo force-pushed uint from 50bc5ee361 to 3ddfd85bf0

2022-03-08 13:43:36 +08:00

Compare

sb10q reviewed 2022-03-08 16:10:04 +08:00

nac3parser/src/token.rs Outdated

						
				@ -112,7 +112,7 @@ impl fmt::Display for Tok {

				        use Tok::*;

				        match self {

				            Name { name } => write!(f, "'{}'", ast::get_str_from_ref(&ast::get_str_ref_lock(), *name)),

				            Int { value } => if let Some(value) = value { write!(f, "'{}'", value) } else { write!(f, "'#OFL#'") },