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

[AMD] ElementwiseOpToLLVM: Do not convert types if they are equal #3091

Merged
merged 5 commits into from
Mar 7, 2024

Conversation

joviliast
Copy link
Contributor

@joviliast joviliast commented Feb 7, 2024

This commit fixes failure in python/tutorials/03-matrix-multiplication.py for FMA cases,
also fixes mixed dot for FMA cases.
Tested on Navi31

@joviliast joviliast requested a review from ptillet as a code owner February 7, 2024 14:49
@binarman
Copy link
Contributor

binarman commented Feb 7, 2024

Essentially, this patch enables triton::FPToFP operation to cast fp16 to fp32 and back, @joviliast is it correct?

@ptillet
Do we want FPToFP operation to be able to convert any float types?

@joviliast
Copy link
Contributor Author

Essentially, this patch enables triton::FPToFP operation to cast fp16 to fp32 and back, @joviliast is it correct?

This patch disables casting. Did you mean "pass"?

@binarman
Copy link
Contributor

binarman commented Feb 7, 2024

This patch disables casting. Did you mean "pass"?

It disables only "intermediate" casting, the rest is in place:

@ptillet
Copy link
Collaborator

ptillet commented Feb 8, 2024

@binarman in principle yes, in practice there are many subtleties related to FP8 and rounding modes. Even our Nvidia backend isn't handling this super well today

@zhanglx13
Copy link
Collaborator

@joviliast @binarman Can you elaborate on the motivation of this PR? Why does converting between the same type cause FMA failures?

@joviliast
Copy link
Contributor Author

@joviliast @binarman Can you elaborate on the motivation of this PR? Why does converting between the same type cause FMA failures?

Because the case of the same internal types just not supported and it fails on compile time as unsupported conversion.

@ThomasRaoux
Copy link
Collaborator

ThomasRaoux commented Feb 22, 2024

@joviliast @binarman Can you elaborate on the motivation of this PR? Why does converting between the same type cause FMA failures?

Because the case of the same internal types just not supported and it fails on compile time as unsupported conversion.

can you make a minimized lit test out of this failure. That will help everybody understand (and will be prevent regressions)

@joviliast
Copy link
Contributor Author

can you make a minimized lit test out of this failure. That will help everybody understand (and will be prevent regressions)

@ThomasRaoux , done.
Added commit containing lit test for fixed case.
It can be ran after merging #3254

Copy link
Collaborator

@ThomasRaoux ThomasRaoux left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for adding a test

@joviliast joviliast force-pushed the fma-matmul branch 2 times, most recently from fce1785 to a69da12 Compare March 6, 2024 14:53
Copy link
Collaborator

@ThomasRaoux ThomasRaoux left a comment

Choose a reason for hiding this comment

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

LGTM

@zhanglx13 zhanglx13 self-requested a review March 6, 2024 17:03
Copy link
Collaborator

@zhanglx13 zhanglx13 left a comment

Choose a reason for hiding this comment

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

lit tests under amd dir is not tested

@ThomasRaoux
Copy link
Collaborator

lit tests under amd dir is not tested

Thanks for catching that. Is someone working on fixing this?

@joviliast
Copy link
Contributor Author

lit tests under amd dir is not tested

I don't quite catch your point.

I can see it passed in CI logs:

screen

This commit fixes failure in python/tutorials/03-matrix-multiplication.py
for FMA cases.

Signed-off-by: joviliast <iveselov.nn@gmail.com>
Signed-off-by: joviliast <iveselov.nn@gmail.com>
@zhanglx13 zhanglx13 self-requested a review March 6, 2024 17:24
Copy link
Collaborator

@zhanglx13 zhanglx13 left a comment

Choose a reason for hiding this comment

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

It actually works. Sorry for blocking the merge.

@zhanglx13 zhanglx13 merged commit 392370b into triton-lang:main Mar 7, 2024
4 checks passed
htyu pushed a commit to htyu/triton that referenced this pull request Mar 20, 2024
…iton-lang#3091)

This commit fixes failure in
python/tutorials/03-matrix-multiplication.py for FMA cases,
also fixes mixed dot for FMA cases.
Tested on Navi31

---------

Signed-off-by: joviliast <iveselov.nn@gmail.com>
binarman pushed a commit to binarman/triton that referenced this pull request Apr 2, 2024
…iton-lang#3091)

This commit fixes failure in
python/tutorials/03-matrix-multiplication.py for FMA cases,
also fixes mixed dot for FMA cases.
Tested on Navi31

---------

Signed-off-by: joviliast <iveselov.nn@gmail.com>
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.

5 participants