core: Change RHS operand of bitshift operators to int32 #349

derppening · 2023-10-31T11:41:47+08:00

derppening commented

2023-10-31 11:41:47 +08:00

Fixes #336.

derppening self-assigned this 2023-10-31 11:41:47 +08:00

derppening force-pushed issue-336 from 9d9d71ffcb to ec350f0b44

2023-11-01 15:01:06 +08:00

Compare

derppening changed title from ~~WIP: core: Change RHS operand of bitshift operators to int32~~ to core: Change RHS operand of bitshift operators to int32

2023-11-01 15:10:32 +08:00

derppening requested review from sb10q 2023-11-01 15:10:36 +08:00

derppening commented

2023-11-01 15:10:47 +08:00

v2: Rebased against master

sb10q commented

2023-11-01 15:19:27 +08:00

Is there a reason why it was done otherwise before?

This will also need changing the code in ARTIQ.

Is there a reason why it was done otherwise before? This will also need changing the code in ARTIQ.

derppening commented

2023-11-01 15:21:35 +08:00

Is there a reason why it was done otherwise before?

This will also need changing the code in ARTIQ.

I have no clue, maybe @pca006132 or @ychenfo can pitch in. I just saw #336 and thought it would be a relatively trivial thing to address.

> Is there a reason why it was done otherwise before? > > This will also need changing the code in ARTIQ. I have no clue, maybe @pca006132 or @ychenfo can pitch in. I just saw #336 and thought it would be a relatively trivial thing to address.

derppening force-pushed issue-336 from ec350f0b44 to 4bdcecdf95

2023-11-01 18:05:35 +08:00

Compare

derppening commented

2023-11-01 18:05:47 +08:00

v3: Rebased against master

sb10q reviewed 2023-11-02 14:00:25 +08:00

nac3core/src/codegen/expr.rs Outdated

						
				@ -303,0 +302,4 @@

				                let rhs = match lhs.get_type().get_bit_width().cmp(&rhs.get_type().get_bit_width()) {

				                    Ordering::Less => self.builder.build_int_truncate(rhs, lhs.get_type(), ""),

				                    Ordering::Equal => rhs,

				                    Ordering::Greater => self.builder.build_int_z_extend(rhs, lhs.get_type(), ""),

sb10q commented

2023-11-02 14:00:24 +08:00

AFAICT this will break silently with negative shifts.

Python disallows them:

>>> 1 << (-1)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: negative shift count

So the RHS type should perhaps be uint32 below instead of int32, and we should check that the compiler reports a sensible error if the user enters a negative value there.

But I worry that uint32 would require explicit casts and a lot of typing, e.g. 1 << uint32(1) instead of just 1 << 1?

Alternatively we can simply support negative shifts since this is just a strict superset of Python as far as I can tell. But the zero-extend here should be replaced with a sign extend.

AFAICT this will break silently with negative shifts. Python disallows them: ``` >>> 1 << (-1) Traceback (most recent call last): File "<stdin>", line 1, in <module> ValueError: negative shift count ``` So the RHS type should perhaps be uint32 below instead of int32, and we should check that the compiler reports a sensible error if the user enters a negative value there. But I worry that uint32 would require explicit casts and a lot of typing, e.g. ``1 << uint32(1)`` instead of just ``1 << 1``? Alternatively we can simply support negative shifts since this is just a strict superset of Python as far as I can tell. But the zero-extend here should be replaced with a sign extend.

derppening force-pushed issue-336 from 4bdcecdf95 to 5d28e4e780

2023-11-02 16:05:01 +08:00

Compare

derppening changed title from ~~core: Change RHS operand of bitshift operators to int32~~ to WIP: core: Change RHS operand of bitshift operators to int32

2023-11-02 16:05:05 +08:00

derppening force-pushed issue-336 from 5d28e4e780 to 09dbae1e63

2023-11-03 16:20:03 +08:00

Compare

derppening commented

2023-11-03 16:20:50 +08:00

v4, v5: Changed bitshift operators to accept both the type of the LHS operand and int32, added compile-time and runtime error checking

v4, v5: Changed bitshift operators to accept both the type of the LHS operand and `int32`, added compile-time and runtime error checking

derppening changed title from ~~WIP: core: Change RHS operand of bitshift operators to int32~~ to core: Change RHS operand of bitshift operators to int32

2023-11-03 16:20:52 +08:00

sb10q commented

2023-11-04 13:39:24 +08:00

accept both the type of the LHS operand and int32

That sounds like unnecessary complications. Why not just int32?

> accept both the type of the LHS operand and int32 That sounds like unnecessary complications. Why not just int32?

derppening commented

2023-11-04 14:10:47 +08:00

accept both the type of the LHS operand and int32

That sounds like unnecessary complications. Why not just int32?

This will cause the operators.py test to require casts on all bitwise shifts, such as:

    a = uint32(17)
    b = uint32(3)

    # All RHS operand of these bitshift operators need to be cast to `int32`

    output_uint32(a << b)
    output_uint32(a >> b)
    
    a <<= b
    a >>= b

It seems to me that there's some redundancy there, as the maximum RHS bitshift value of 0..63 can be represented by any integral type supported by NAC3.

> > accept both the type of the LHS operand and int32 > > That sounds like unnecessary complications. Why not just int32? This will cause the `operators.py` test to require casts on all bitwise shifts, such as: ```py a = uint32(17) b = uint32(3) # All RHS operand of these bitshift operators need to be cast to `int32` output_uint32(a << b) output_uint32(a >> b) a <<= b a >>= b ``` It seems to me that there's some redundancy there, as the maximum RHS bitshift value of `0..63` can be represented by any integral type supported by NAC3.

sb10q commented

2023-11-04 14:13:56 +08:00

This will cause the operators.py test to require casts on all bitwise shifts

Sounds fine to me. It's just a bit of simple code into one test.

> This will cause the operators.py test to require casts on all bitwise shifts Sounds fine to me. It's just a bit of simple code into one test.

sb10q commented

2023-11-04 14:14:19 +08:00

And it makes it clear to someone reading the test that nac3 expects int32 there.

derppening commented

2023-11-04 14:21:56 +08:00

After changing it to cast to int32 I got this error:

Checking src/operators.py... Traceback (most recent call last):
  File "/home/david/programming/nac3/nac3standalone/demo/./interpret_demo.py", line 186, in <module>
    main()
  File "/home/david/programming/nac3/nac3standalone/demo/./interpret_demo.py", line 182, in main
    demo.run()
  File "src/operators.py", line 23, in run
    test_uint64()
  File "src/operators.py", line 146, in test_uint64
    output_uint64(a << int32(b))
                  ~~^^~~~~~~~~~
TypeError: ufunc 'left_shift' not supported for the input types, and the inputs could not be safely coerced to any supported types according to the casting rule ''safe''

It seems like NumPy doesn't allow the bitshift operator with uint64 and int32 as its types, all other cases (int32/int64/uint32) work.

After changing it to cast to int32 I got this error: ``` Checking src/operators.py... Traceback (most recent call last): File "/home/david/programming/nac3/nac3standalone/demo/./interpret_demo.py", line 186, in <module> main() File "/home/david/programming/nac3/nac3standalone/demo/./interpret_demo.py", line 182, in main demo.run() File "src/operators.py", line 23, in run test_uint64() File "src/operators.py", line 146, in test_uint64 output_uint64(a << int32(b)) ~~^^~~~~~~~~~ TypeError: ufunc 'left_shift' not supported for the input types, and the inputs could not be safely coerced to any supported types according to the casting rule ''safe'' ``` It seems like NumPy doesn't allow the bitshift operator with `uint64` and `int32` as its types, all other cases (`int32`/`int64`/`uint32`) work.

sb10q commented

2023-11-04 14:26:44 +08:00

Yes this can be reproduced easily:

>>> import numpy as np
>>> np.uint64(1) << np.int32(1)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: ufunc 'left_shift' not supported for the input types, and the inputs could not be safely coerced to any supported types according to the casting rule ''safe''

Yes this can be reproduced easily: ``` >>> import numpy as np >>> np.uint64(1) << np.int32(1) Traceback (most recent call last): File "<stdin>", line 1, in <module> TypeError: ufunc 'left_shift' not supported for the input types, and the inputs could not be safely coerced to any supported types according to the casting rule ''safe'' ```

sb10q commented

2023-11-04 14:28:20 +08:00

Also without int32:

>>> np.uint64(1) << 1
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: ufunc 'left_shift' not supported for the input types, and the inputs could not be safely coerced to any supported types according to the casting rule ''safe''

Also without int32: ``` >>> np.uint64(1) << 1 Traceback (most recent call last): File "<stdin>", line 1, in <module> TypeError: ufunc 'left_shift' not supported for the input types, and the inputs could not be safely coerced to any supported types according to the casting rule ''safe''

derppening commented

2023-11-04 14:29:33 +08:00

Apart from allowing both int32 and the LHS operand type, do you have other suggestions on how to workaround this issue?

Apart from allowing both `int32` and the LHS operand type, do you have other suggestions on how to workaround this issue?

sb10q commented

2023-11-04 14:30:25 +08:00

Let's try to understand first why numpy does it like this, seems it's not just a numpy bug.

sb10q commented

2023-11-04 14:32:47 +08:00

allowing both int32 and the LHS operand type

And I don't see how this resolves anything (other than hide the issue somewhat in the test) since nac3 would still allow np.uint64(1) << 1 but not numpy.

> allowing both int32 and the LHS operand type And I don't see how this resolves anything (other than hide the issue somewhat in the test) since nac3 would still allow `np.uint64(1) << 1` but not numpy.

derppening commented

2023-11-04 14:38:18 +08:00

It seems like this issue is documented here: https://github.com/numpy/numpy/issues/22624 and https://github.com/numpy/numpy/issues/2524.

This is because the shift number is converted as a signed type and there is no signed integer type big enough to hold a uint64.

It seems like this issue is documented here: https://github.com/numpy/numpy/issues/22624 and https://github.com/numpy/numpy/issues/2524. > This is because the shift number is converted as a signed type and there is no signed integer type big enough to hold a uint64.

sb10q commented

2023-11-04 14:42:02 +08:00

np._set_promotion_state("weak") may be a fix (just like we already need from __future__ import annotations)
https://numpy.org/neps/nep-0050-scalar-promotion.html

`np._set_promotion_state("weak")` may be a fix (just like we already need `from __future__ import annotations`) https://numpy.org/neps/nep-0050-scalar-promotion.html

derppening commented

2023-11-06 12:19:50 +08:00

>>> import numpy as np
>>> np._get_promotion_state()
'legacy'
>>> np._set_promotion_state("weak")
>>> np._get_promotion_state()
'weak'
>>> np.uint64(1) << np.int32(1)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: ufunc 'left_shift' not supported for the input types, and the inputs could not be safely coerced to any supported types according to the casting rule ''safe''

How about directly hardcoding the behavior of NumPy, i.e. allowing int32 RHS operands to int32/uint32/int64 RHS operands but only allowing uint64 for uint64 LHS operand?

``` >>> import numpy as np >>> np._get_promotion_state() 'legacy' >>> np._set_promotion_state("weak") >>> np._get_promotion_state() 'weak' >>> np.uint64(1) << np.int32(1) Traceback (most recent call last): File "<stdin>", line 1, in <module> TypeError: ufunc 'left_shift' not supported for the input types, and the inputs could not be safely coerced to any supported types according to the casting rule ''safe'' ``` How about directly hardcoding the behavior of NumPy, i.e. allowing `int32` RHS operands to `int32`/`uint32`/`int64` RHS operands but only allowing `uint64` for `uint64` LHS operand?

sb10q commented

2023-11-06 12:38:17 +08:00

Doesn't seem like people are happy with the numpy behavior as evidenced by the links above. Having int32 only is a simple solution which AFAICT alleviates legitimate concerns, and if numpy acts up simply use _set_promotion_state to get the behavior that numpy are planning to implement.

Doesn't seem like people are happy with the numpy behavior as evidenced by the links above. Having int32 only is a simple solution which AFAICT alleviates legitimate concerns, and if numpy acts up simply use ``_set_promotion_state`` to get the behavior that numpy are planning to implement.

sb10q commented

2023-11-06 12:39:11 +08:00

And this will only matter in @portable code that shifts unsigned integers i.e. a minor use case.

And this will only matter in ``@portable`` code that shifts unsigned integers i.e. a minor use case.

derppening commented

2023-11-06 12:40:55 +08:00

if numpy acts up simply use _set_promotion_state to get the behavior that numpy are planning to implement.

The issue right now is that _set_promotion_state doesn't seem to affect bitwise shift operators as evident from the interpreter snippet.

> if numpy acts up simply use `_set_promotion_state` to get the behavior that numpy are planning to implement. The issue right now is that `_set_promotion_state` doesn't seem to affect bitwise shift operators as evident from the interpreter snippet.

sb10q commented

2023-11-06 12:44:04 +08:00

Actually it does but incompletely...

>>> import numpy as np
>>> np.uint64(1) << np.int32(1)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: ufunc 'left_shift' not supported for the input types, and the inputs could not be safely coerced to any supported types according to the casting rule ''safe''
>>> np.uint64(1) << 1          
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: ufunc 'left_shift' not supported for the input types, and the inputs could not be safely coerced to any supported types according to the casting rule ''safe''
>>> np._set_promotion_state("weak")
>>> np.uint64(1) << np.int32(1)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: ufunc 'left_shift' not supported for the input types, and the inputs could not be safely coerced to any supported types according to the casting rule ''safe''
>>> np.uint64(1) << 1          
2

Actually it does but incompletely... ``` >>> import numpy as np >>> np.uint64(1) << np.int32(1) Traceback (most recent call last): File "<stdin>", line 1, in <module> TypeError: ufunc 'left_shift' not supported for the input types, and the inputs could not be safely coerced to any supported types according to the casting rule ''safe'' >>> np.uint64(1) << 1 Traceback (most recent call last): File "<stdin>", line 1, in <module> TypeError: ufunc 'left_shift' not supported for the input types, and the inputs could not be safely coerced to any supported types according to the casting rule ''safe'' >>> np._set_promotion_state("weak") >>> np.uint64(1) << np.int32(1) Traceback (most recent call last): File "<stdin>", line 1, in <module> TypeError: ufunc 'left_shift' not supported for the input types, and the inputs could not be safely coerced to any supported types according to the casting rule ''safe'' >>> np.uint64(1) << 1 2 ```

sb10q commented

2023-11-06 12:50:09 +08:00

Maybe we can support both int32 and uint32?

Then:

we can still write "x << 2" instead of "x << uint32(2)".
if it's already uint32, the exception test can be removed and the code accelerated.
numpy compatibility remains always possible, at the cost of requiring _set_promotion_state and/or verbose type conversions.

Maybe we can support both int32 and uint32? Then: * we can still write "x << 2" instead of "x << uint32(2)". * if it's already uint32, the exception test can be removed and the code accelerated. * numpy compatibility remains always possible, at the cost of requiring `_set_promotion_state` and/or verbose type conversions.

derppening commented

2023-11-06 14:12:02 +08:00

Maybe we can support both int32 and uint32?

Then:

we can still write "x << 2" instead of "x << uint32(2)"

if it's already uint32, the exception test can be removed and the code accelerated

numpy compatibility remains always possible, even at the cost of requiring _set_promotion_state and/or verbose type conversions.

How about we only accept int32/uint32 depending on the signness of the left operand? It seems like NAC3's unifier does not support overloading for the same operator.

> Maybe we can support both int32 and uint32? > > Then: > * we can still write "x << 2" instead of "x << uint32(2)" > * if it's already uint32, the exception test can be removed and the code accelerated > * numpy compatibility remains always possible, even at the cost of requiring `_set_promotion_state` and/or verbose type conversions. > How about we only accept `int32`/`uint32` depending on the signness of the left operand? It seems like NAC3's unifier does not support overloading for the same operator.

derppening force-pushed issue-336 from 09dbae1e63 to 685475c6f5

2023-11-06 14:57:26 +08:00

Compare

derppening commented

2023-11-06 15:02:34 +08:00

v6: Rebased against master, require bitshift operands to match in sign-ness.

sb10q commented

2023-11-06 20:15:02 +08:00

How about we only accept int32/uint32 depending on the signness of the left operand?

Does this make any sense from the user's point of view?

> How about we only accept int32/uint32 depending on the signness of the left operand? Does this make any sense from the user's point of view?

derppening commented

2023-11-07 18:46:17 +08:00

Honestly speaking, I don't really see a good solution for this issue without having to extend the type inferencer to support function overloads or union types...

Requiring matching signness is probably the best solution without having to mess with the inferencer.

Honestly speaking, I don't really see a good solution for this issue without having to extend the type inferencer to support function overloads or union types... Requiring matching signness is probably the best solution without having to mess with the inferencer.

derppening force-pushed issue-336 from 685475c6f5 to 856b4fb371

2023-11-08 17:22:40 +08:00

Compare

derppening commented

2023-11-08 17:23:08 +08:00

v7: Allow only int32 and uint32 on RHS operand of bitshift operator

v7: Allow only `int32` and `uint32` on RHS operand of bitshift operator

sb10q reviewed 2023-11-08 17:32:03 +08:00

nac3core/src/codegen/expr.rs Outdated

						
				@ -302,1 +301,3 @@

				            (Operator::RShift, _) => self.builder.build_right_shift(lhs, rhs, true, "rshift").into(),

				            (Operator::LShift, signed) | (Operator::RShift, signed) => {

				                let common_type = lhs.get_type();

				                let rhs = if common_type.get_bit_width() > rhs.get_type().get_bit_width() {

sb10q commented

2023-11-08 17:32:03 +08:00

RHS is always 32, no?

sb10q reviewed 2023-11-08 17:34:08 +08:00

nac3core/src/codegen/expr.rs Outdated

						
				@ -303,0 +307,4 @@

				                        self.builder.build_int_z_extend(rhs, common_type, "")

				                    }

				                } else {

				                    self.builder.build_int_truncate_or_bit_cast(rhs, common_type, "")

sb10q commented

2023-11-08 17:34:08 +08:00

Do we ever do this? Are there any int types smaller than 32-bit?

sb10q reviewed 2023-11-08 17:34:47 +08:00

nac3core/src/codegen/expr.rs Outdated

						
				@ -303,0 +310,4 @@

				                    self.builder.build_int_truncate_or_bit_cast(rhs, common_type, "")

				                };

				                let rhs_gez = self.builder.build_int_compare(IntPredicate::SGE, rhs, common_type.const_zero(), "");

sb10q commented

2023-11-08 17:34:47 +08:00

Is LLVM able to optimize this away when the RHS is uint32?

sb10q commented

2023-11-08 17:35:05 +08:00

Or a constant?

derppening commented

2023-11-09 11:50:34 +08:00

Yes, all test cases can be constant-folded using optimizations.

sb10q reviewed 2023-11-08 17:38:35 +08:00

nac3core/src/codegen/expr.rs Outdated

						
				@ -300,3 +300,2 @@

				            (Operator::BitAnd, _) => self.builder.build_and(lhs, rhs, "and").into(),

				            (Operator::LShift, _) => self.builder.build_left_shift(lhs, rhs, "lshift").into(),

				            (Operator::RShift, _) => self.builder.build_right_shift(lhs, rhs, true, "rshift").into(),

				            (Operator::LShift, signed) | (Operator::RShift, signed) => {