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

stablehlo.compare integer comparison compare_type inconsistencies #2354

Open
christopherbate opened this issue May 17, 2024 · 0 comments
Open

Comments

@christopherbate
Copy link
Contributor

christopherbate commented May 17, 2024

What happened?

I was looking at this rewrite pattern for CompareOp in stablehlo-aggressive-simplification here, and there is a comment that seems to indicate that all stablehlo.compare operations on integers should have either a 'SIGNED' or 'UNSIGNED' comparison type (i.e. the Attribute returned by op.getComparisonType() should not be empty).

However, this seems overly aggressive since after looking at the operations ODS specification, the "compare_type" Attribute can be omitted (even if the operands are integers and the "compare_direction" is LT|GT|LE|GE ).

I guess that one is supposed to interpret stablehlo.compare LT %0, %1 : (tensor<10xi32>, tensor<10xi32>) -> tensor<10xi1> as being SIGNED? If so, maybe the parser should be updated to reflect that (or add a method like CompareOp::getComparisonDirectionOrDefault).

The spec says:

(C3) compare_type is defined as:
- SIGNED if is_signed_integer(element_type(lhs)).
- UNSIGNED if is_unsigned_integer(element_type(lhs)) or is_boolean(element_type(lhs)).
- FLOAT or TOTALORDER if is_float(element_type(lhs)).
- FLOAT if is_complex(element_type(lhs)).

It makes no mention of signless even though MLIR refers to i32, i64, etc as signless integers in its APIs. The spec doesn't note this anywhere which seems odd. It only says integers should be uiX or siX. However use of iX appears prevalent in the tests and examples.

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

No branches or pull requests

1 participant