M-Labs
/
nac3
8
0
Fork 5
Code Issues 87 Pull Requests 3 Releases Wiki Activity

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

Merged
sb10q merged 2 commits from issue-336 into master 2023-11-09 12:16:21 +08:00
Conversation 29 Commits 2 Files Changed 6
derppening commented 2023-10-31 11:41:47 +08:00
Collaborator

Fixes #336.

Fixes #336.
derppening self-assigned this 2023-10-31 11:41:47 +08:00
derppening added 1 commit 2023-10-31 11:41:48 +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
Poster
Collaborator

v2: Rebased against master

v2: Rebased against master
sb10q commented 2023-11-01 15:19:27 +08:00
Owner

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
Poster
Collaborator

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
Poster
Collaborator

v3: Rebased against master

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
Owner

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
Poster
Collaborator

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
Owner

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
Poster
Collaborator

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
Owner

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
Owner

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

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
Poster
Collaborator

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
Owner

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
Owner

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
Poster
Collaborator

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
Owner

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

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
Owner

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
Poster
Collaborator

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
Owner

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
Poster
Collaborator 
>>> 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
Owner

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
Owner

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
Poster
Collaborator

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
Owner

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
Owner

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
Poster
Collaborator

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
Poster
Collaborator

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

v6: Rebased against master, require bitshift operands to match in sign-ness.
sb10q commented 2023-11-06 20:15:02 +08:00
Owner

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
Poster
Collaborator

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
Poster
Collaborator

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
Owner

RHS is always 32, no?

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
Owner

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

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
Owner

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

Is LLVM able to optimize this away when the RHS is uint32?
sb10q commented 2023-11-08 17:35:05 +08:00
Owner

Or a constant?

Or a constant?
derppening commented 2023-11-09 11:50:34 +08:00
Poster
Collaborator

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

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) => {
sb10q commented 2023-11-08 17:38:35 +08:00
Owner

It's become unclear here to which side of the operand signed refers to. I suggest adding a comment here to clarify what is going on.

It's become unclear here to which side of the operand ``signed`` refers to. I suggest adding a comment here to clarify what is going on.
derppening force-pushed issue-336 from 856b4fb371 to 62790ce974 2023-11-09 11:50:12 +08:00 Compare
derppening commented 2023-11-09 11:51:07 +08:00
Poster
Collaborator

v8: Minor code cleanup

v8: Minor code cleanup
sb10q merged commit d322c91697 into master 2023-11-09 12:16:20 +08:00
sb10q deleted branch issue-336 2023-11-09 12:16:23 +08:00
Sign in to join this conversation.
Reviewers
No reviewers
sb10q
Labels
Clear labels   
UROP0622
   
future-extension
   
high-priority
   
low-priority
No Label
UROP0622
future-extension
high-priority
low-priority
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
derppening
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: M-Labs/nac3#349
There is no content yet.
Delete Branch "issue-336"

Deleting a branch is permanent. Although the deleted branch may exist for a short time before cleaning up, in most cases it CANNOT be undone. Continue?