I was looking at this rewrite pattern for CompareOp in stablehlo-aggressive-simplificationhere, 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.
What happened?
I was looking at this rewrite pattern for
CompareOpinstablehlo-aggressive-simplificationhere, and there is a comment that seems to indicate that allstablehlo.compareoperations on integers should have either a 'SIGNED' or 'UNSIGNED' comparison type (i.e. theAttributereturned byop.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 beingSIGNED? If so, maybe the parser should be updated to reflect that (or add a method likeCompareOp::getComparisonDirectionOrDefault).The spec says:
It makes no mention of
signlesseven though MLIR refers toi32,i64, etc as signless integers in its APIs. The spec doesn't note this anywhere which seems odd. It only says integers should beuiXorsiX. However use ofiXappears prevalent in the tests and examples.