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

Decide on corner cases for rhs in ShiftXxxOps #1150

Open
wecing opened this issue Feb 10, 2023 · 2 comments
Open

Decide on corner cases for rhs in ShiftXxxOps #1150

wecing opened this issue Feb 10, 2023 · 2 comments
Labels

Comments

@wecing
Copy link

wecing commented Feb 10, 2023

Request description

For example, in C++ (and C89), if the value of the right operand is negative or is greater or equal to the number of bits in the promoted left operand, the behavior is undefined.

Should we specify the same restriction for StableHLO?

@burmako burmako added the Spec label Feb 10, 2023
@burmako burmako changed the title Shift operations should restrict offset value Decide on corner cases for rhs in ShiftXxxOps Feb 11, 2023
@burmako
Copy link
Contributor

burmako commented Feb 11, 2023

That's a nice catch - thank you for calling this out! This question has also come up on the #stablehlo Discord recently.

When writing the initial version of the spec, we've abstracted ourselves away from numerical corner cases, stating that "Error conditions are [...] possible, e.g. through integer overflows, out-of-bounds accesses, etc. Unless explicitly called out, all these errors result in implementation-defined behavior. [However] as an exception to this rule, floating-point exceptions in StableHLO programs have well-defined behavior [...] defined by the IEEE-754 standard".

This was convenient for the initial version, but this way we not only deferred figuring out error conditions (desired effect) but also missed some interesting corner cases like this one (undesired effect). Before we release StableHLO v1.0, it would be good to audit the opset and address both items - I opened a ticket to follow up on that: #1157.

For this specific ticket, how urgent is it on your side? The easiest way to handle it would be as part of the aforementioned audit sometime later this year. However, if there's a need in faster turnaround, we can also try to find time for that.

@wecing
Copy link
Author

wecing commented Feb 13, 2023

Thanks for the info!

For this specific ticket, how urgent is it on your side?

Not urgent at all, I would say.

We do not have any existing use cases for these ops; I just ran into this corner case while trying to provide full StableHLO support in our system.

ghpvnist added a commit that referenced this issue Apr 20, 2023
Here are the constraints for the ShiftLeftOp:
```
(I1) lhs is a tensor of integer type.
(I2) rhs is a tensor of integer type.
(C1) `lhs`, `rhs`, and `result` have the same type.
```
I1, I2, and C1 are covered by the ODS, so no additional tests are added.

Notes:
* Corner cases (shift overflow) has not been accounted for: #1150

closes #1112
ghpvnist added a commit that referenced this issue Apr 20, 2023
Here are the constraints for the ShiftRightLogicalOp:
```
(I1) lhs is a tensor of integer type.
(I2) rhs is a tensor of integer type.
(C1) `lhs`, `rhs`, and `result` have the same type.
```
I1, I2, and C1 are covered by the ODS, so no additional tests are added.

Notes:
* Corner cases (shift overflow) has not been accounted for: #1150

closes #1114
ghpvnist added a commit that referenced this issue Apr 20, 2023
Here are the constraints for the ShiftRightArithmeticOp:
```
(I1) lhs is a tensor of integer type.
(I2) rhs is a tensor of integer type.
(C1) `lhs`, `rhs`, and `result` have the same type.
```
I1, I2, and C1 are covered by the ODS, so no additional tests are added.

Notes:
* Corner cases (shift overflow) has not been accounted for: #1150

closes #1113
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: No status
Development

No branches or pull requests

2 participants