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

[AMP] refine AMP and the corresponding tests for bfloat16 #12787

Merged
merged 3 commits into from
Oct 27, 2022

Conversation

yangulei
Copy link
Contributor

@yangulei yangulei commented Sep 15, 2022

This PR fixes issue #12763, where some OP are marked to keep the original dtype but some of its input is bfloat16 while a Cast is missing.
The AMP tests have also been refined to cover bfloat16 without accuracy checking.

Update:
The accuracy checking in test_dnnl.py of bf16 vs fp32 is unstable and error-prone. Thus the accuracy checking is ignored if only one bf16 result present, i.e. only compare bf16 vs bf16 and fp32 vs fp32.

@billishyahao
Copy link
Contributor

@tvm-bot rerun

@billishyahao
Copy link
Contributor

Thanks for the patch, Youlei! I found a bunch of statement like "op->dtype.is_float() || op->dtype.is_bfloat16()" in tvm folder

Shall we simply add new float type definition in tvm/include/tvm/runtime/data_type.h to eliminate those statements?

  /*! \return whether type is a general float type, including float/float16/bfloat16. */
  bool is_general_float() const { return is_float() || is_bfloat16(); }

@yangulei
Copy link
Contributor Author

@billishyahao
I agree that is_general_float() could make the code cleaner, but not clearer. general float is a broader concept rather than IEEE float point plus bfloat16, for example, TensorFloat-32 is also a general float. I prefer to keep the expr like op->dtype.is_float() || op->dtype.is_bfloat16() as its more clear and specific.

@areusch areusch added needs-triage PRs or issues that need to be investigated by maintainers to find the right assignees to address it and removed needs-triage PRs or issues that need to be investigated by maintainers to find the right assignees to address it labels Oct 19, 2022
@yangulei
Copy link
Contributor Author

@masahi Could you help to review this? Thanks.

@masahi masahi merged commit 5c9066d into apache:main Oct 27, 2022
xinetzone pushed a commit to daobook/tvm that referenced this pull request Nov 10, 2022
* refine AMP for bfloat16

* refine AMP tests to cover bfloat16

* refine accuracy checking for dnnl bf16
xinetzone pushed a commit to daobook/tvm that referenced this pull request Nov 25, 2022
* refine AMP for bfloat16

* refine AMP tests to cover bfloat16

* refine accuracy checking for dnnl bf16
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