Skip to content

[TIR] Autocorrect Range min and extent types when possible#15872

Closed
kparzysz-quic wants to merge 13 commits intoapache:mainfrom
kparzysz-quic:autocorrect-range-types
Closed

[TIR] Autocorrect Range min and extent types when possible#15872
kparzysz-quic wants to merge 13 commits intoapache:mainfrom
kparzysz-quic:autocorrect-range-types

Conversation

@kparzysz-quic
Copy link
Copy Markdown
Contributor

The values of min and extent in Range should have the same type, but the usage of convenience may often produce type mismatches, for example Range(0, dim) will assume int32 for the min. These mismatches can then cause hard-to-track errors, in particular assertions in the arithmetic simplifier.
This is already done for For statement, but not in other places.

This patch introduces DataType::WidestOf function, that returns the widest type from the given list. It is then used to promote the integer bounds if such widest type exists.

Krzysztof Parzyszek added 2 commits October 3, 2023 17:47
The values of `min` and `extent` in Range should have the same type,
but the usage of convenience may often produce type mismatches, for
example `Range(0, dim)` will assume int32 for the `min`.
These mismatches can then cause hard-to-track errors, in particular
assertions in the arithmetic simplifier.
This is already done for `For` statement, but not in other places.

This patch introduces `DataType::WidestOf` function, that returns
the widest type from the given list. It is then used to promote the
integer bounds if such widest type exists.
@tqchen
Copy link
Copy Markdown
Member

tqchen commented Oct 4, 2023

In most cases the intention was to construct IRs that have consistent types. So insert casting like cast(i32_val + 1, i64) is still not desirable for cases like arith simplifier. Ideally the caller should directly construct the min, extent with the same type, or only do type conversion when certain value is constant.

How about we do the following things:

  • Auto promote trivial constants, in which case no cast will be introduced
  • Error out(instead of silently promote) in constructor for cases that are in-consistent, so we can have callers to fix the type tracking when possible.

Comment thread include/tvm/runtime/data_type.h Outdated
* hold all values representable in the other types, or "Void" if
* such type is not present in the array.
*/
static DataType WidestOf(const std::vector<DataType>& types) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it possible to use template trick or overloading to handle two/three case, mainly avoid the vector passing

Copy link
Copy Markdown
Contributor Author

@kparzysz-quic kparzysz-quic Oct 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would std::initializer_list be ok?

Edit: Nevermind, I'll do the template.

Krzysztof Parzyszek added 3 commits October 4, 2023 10:55
Test failing in CI:
```
a = T.Cast("int8", T.tvm_struct_get(args, 0, 12, "int64"))
b = 1
Range(a, b)
```

The value 1 has a type int32 by default, but since we're avoiding extra
casts, the only way to legalize this range is by reducing the extent
to int8.
@kparzysz-quic
Copy link
Copy Markdown
Contributor Author

I'm not sure what you think. This is adding a lot of code for a simple thing, but I'm hoping that the extra functions may be useful in other cases as well.

@kparzysz-quic
Copy link
Copy Markdown
Contributor Author

This cannot be done correctly in Range alone. This is because there are cases like Bind(var, Range(a, b)). If a and b are both immediates of different types, then there is no way to infer the correct common type, because the correct type is that of the var, which is not known to the range.

@kparzysz-quic kparzysz-quic deleted the autocorrect-range-types branch October 10, 2023 15:16
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

Successfully merging this pull request may close these issues.

2 participants