Skip to content

Conversation

@masahi
Copy link
Member

@masahi masahi commented Aug 4, 2021

Right now running the fp16 conversion pass on arange op results in a type error fp16 vs fp32, since we need to update dtype attributes of arange

DataType dtype;

We could fix the type error that way and support fp16 arguments to arange, but that would cause a problem when end argument is a large number which happens to be fp32. So we should disallow fp16 conversion for arange. Since the inputs to arange are scalars, cast overhead is negligible.
@AndrewZhaoLuo

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, although this does scarify GPU performance as you mentioned in the issue. Maybe we could rethink the way specifying whether an op can be casted or not. For example, we could have target-specific lists that can be used when running this pass with a target.

@AndrewZhaoLuo
Copy link
Contributor

LGTM, I would add a comment explaining why it's a red listed operation (e.g. end is a large number).

Copy link
Member

@junrushao junrushao left a comment

Choose a reason for hiding this comment

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

Thanks Masa!

@masahi masahi merged commit 0ce7f6c into apache:main Aug 4, 2021
mehrdadh pushed a commit to mehrdadh/tvm that referenced this pull request Aug 11, 2021
* [AMP] Do not allow fp16 cast on arange inputs

* add test

* Add comment explaining the issue with fp16 "end"
ylc pushed a commit to ylc/tvm that referenced this pull request Sep 29, 2021
* [AMP] Do not allow fp16 cast on arange inputs

* add test

* Add comment explaining the issue with fp16 "end"
ylc pushed a commit to ylc/tvm that referenced this pull request Jan 13, 2022
* [AMP] Do not allow fp16 cast on arange inputs

* add test

* Add comment explaining the issue with fp16 "end"
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.

4 participants