Skip to content

Conversation

@AndrewZhaoLuo
Copy link
Contributor

@AndrewZhaoLuo AndrewZhaoLuo commented Jul 20, 2021

Right now if we in TIR add a floating point type with an integer type, the integer type will be implicitly cast to the floating point type.

E.g. a: float32 + b: int32 ---> a: float32 + cast(float32, b: int32)

This change does the same thing for floating point types. If we have two different floating point types e.g. fp16 and fp32, then when operating, the lower bit floating point type gets cast into the higher bit floating point type.

E.g. a: float32 + b: float16 --> a: float32 + cast(float32, b: float16)

This is of use since #8340 has an issue where some schedules which should support mixed precision types do not. This is due to binary ops like addition and multiplication not supporting mixing fp32 and fp16. Most of these errors can be fixed by inserting a cast into the schedule.

Rather than manually audit every schedule which might have this, this might be a preferable and reasonable solution.

@AndrewZhaoLuo AndrewZhaoLuo changed the title [TIR] upcast disparate floating point types for binary ops [TIR] cast disparate floating point types for binary ops Jul 20, 2021
@AndrewZhaoLuo
Copy link
Contributor Author

AndrewZhaoLuo commented Jul 20, 2021

Thanks for the review @comaniac PTAL

@AndrewZhaoLuo
Copy link
Contributor Author

By the way, please don't merge this anytime soon. I want to get some input from some others like @jroesch or @junrushao1994

@AndrewZhaoLuo AndrewZhaoLuo changed the title [TIR] cast disparate floating point types for binary ops [Do not merge!] [TIR] cast disparate floating point types for binary ops Jul 20, 2021
Copy link
Contributor

@comaniac comaniac left a comment

Choose a reason for hiding this comment

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

LGTM. Leave it to @jroesch @junrushao1994

@jroesch
Copy link
Member

jroesch commented Jul 23, 2021

As long as we don't implicitly downcast I think this should be fine, can you test behavior of storing both an i32/u32 computation in i16/u16, and the other way around?

@AndrewZhaoLuo
Copy link
Contributor Author

AndrewZhaoLuo commented Jul 23, 2021

As long as we don't implicitly downcast I think this should be fine, can you test behavior of storing both an i32/u32 computation in i16/u16, and the other way around?

This will fail since this PR only upcasts at most one arg. in binary operations only (e.g. addition). Do you want me to also upcast assignment for floating point types to be consistent? @jroesch

@AndrewZhaoLuo AndrewZhaoLuo changed the title [Do not merge!] [TIR] cast disparate floating point types for binary ops [TIR] cast disparate floating point types for binary ops Jul 23, 2021
@jroesch
Copy link
Member

jroesch commented Jul 27, 2021

As long as we don't implicitly downcast I think this should be fine, can you test behavior of storing both an i32/u32 computation in i16/u16, and the other way around?

This will fail since this PR only upcasts at most one arg. in binary operations only (e.g. addition). Do you want me to also upcast assignment for floating point types to be consistent? @jroesch

I meant we should add negative tests so that if someone later comes along to modify the behavior we have clearly written down what should pass and what should fail.

@AndrewZhaoLuo
Copy link
Contributor Author

As long as we don't implicitly downcast I think this should be fine, can you test behavior of storing both an i32/u32 computation in i16/u16, and the other way around?

This will fail since this PR only upcasts at most one arg. in binary operations only (e.g. addition). Do you want me to also upcast assignment for floating point types to be consistent? @jroesch

I meant we should add negative tests so that if someone later comes along to modify the behavior we have clearly written down what should pass and what should fail.

Oh ok. Done.

@AndrewZhaoLuo
Copy link
Contributor Author

PTAL @jroesch

@jroesch
Copy link
Member

jroesch commented Jul 28, 2021

LGTM, just get it green and we are gtg

@comaniac comaniac merged commit 66b3cc9 into apache:main Jul 29, 2021
@comaniac
Copy link
Contributor

Thanks @AndrewZhaoLuo @jroesch

ylc pushed a commit to ylc/tvm that referenced this pull request Sep 29, 2021
* handle upcasting case

* test upcasting tests for tir

* address comaniac comments

* formatting

* add negative tests

* fix failing test now allow other things

Co-authored-by: Andrew Zhao Luo <andrewzhaoluo@system76-pc.localdomain>
ylc pushed a commit to ylc/tvm that referenced this pull request Jan 13, 2022
* handle upcasting case

* test upcasting tests for tir

* address comaniac comments

* formatting

* add negative tests

* fix failing test now allow other things

Co-authored-by: Andrew Zhao Luo <andrewzhaoluo@system76-pc.localdomain>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants