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

MIOpenTensile Part2 : Support low precision types #556

Merged
merged 166 commits into from
Apr 18, 2021
Merged

Conversation

ce1adon
Copy link
Contributor

@ce1adon ce1adon commented Oct 29, 2020

The purpose of this PR is to add MIOpenTensile path in GEMM algorithm for low precision types on hip backend.
This PR depends on #139. Set "on hold" till #139 merged.
Update: The targetID env changes are in PR #674

@ce1adon
Copy link
Contributor Author

ce1adon commented Apr 15, 2021

@atamazov @DrizztDoUrden @asroy @pfultz2 @JehandadKhan
Final call for comments please

@atamazov
Copy link
Contributor

We are suppose to have one stage ON.

I thought we agreed that MIOpenTensile tests will be OFF by default. Could you please remind why do you need to always test "Latest"?

/cc @pfultz2 @JehandadKhan

@atamazov
Copy link
Contributor

@ce1adon I would like to merge this today; this question can be postponed for a day or two, I guess. But let me do some final fixes before merging -- that would be faster than discussions.

@ce1adon
Copy link
Contributor Author

ce1adon commented Apr 16, 2021

We are suppose to have one stage ON.

I thought we agreed that MIOpenTensile tests will be OFF by default. Could you please remind why do you need to always test "Latest"?

/cc @pfultz2 @JehandadKhan

@atamazov In our meeting, My original proposal was to disable both tensile stages. Paul @pfultz2 mentioned we have to keep one tensile test stage in the CI to justify other PRs. Here I leave the latest tensile version stage ON.

@atamazov
Copy link
Contributor

@ce1adon

@atamazov In our meeting, My original proposal was to disable both tensile stages. Paul @pfultz2 mentioned we have to keep one tensile test stage in the CI to justify other PRs. Here I leave the latest tensile version stage ON.

What if I add some smoke tests for Tensile and keep them always enabled, but disable all Full MIOpenTensile tests by default? That would save ~1.5-2 hours for each CI job.

@ce1adon
Copy link
Contributor Author

ce1adon commented Apr 16, 2021

@ce1adon

@atamazov In our meeting, My original proposal was to disable both tensile stages. Paul @pfultz2 mentioned we have to keep one tensile test stage in the CI to justify other PRs. Here I leave the latest tensile version stage ON.

What if I add some smoke tests for Tensile and keep them always enabled, but disable all Full MIOpenTensile tests by default? That would save ~1.5-2 hours for each CI job.

I've already disabled all the tests I can and I didn't even enable conv3d for gfx908 as I recommended in the meeting. You are welcome to prune the test into a new smoke test in a new PR later. For this one I just hope it can be wrapped. The longer we wait, the more merge conflicts will emerge.

@@ -188,11 +188,11 @@ pipeline {
description: "")
booleanParam(
name: "MIOPENTENSILE",
defaultValue: off,
defaultValue: false,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What's the point to test this PR for so long if both tensile stages are disable?

Copy link
Contributor

Choose a reason for hiding this comment

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

Everyone can manually enable tests when needed:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's OK if you want to expand it to a new feature. But please aware that this PR is currently a blocker of many consecutive tasks and PRs. Please treat it as high priority and urgent as the tag says.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am ready to merge it as soon as CI testing done (if no problems). However, our CI is overloaded with other jobs, including unnecessary ones initiated by DevOps.

@atamazov
Copy link
Contributor

You are welcome to prune the test into a new smoke test in a new PR later.

Ok, the "Latest" stage is enabled back. I'll prepare separate PR.

atamazov
atamazov previously approved these changes Apr 16, 2021
@atamazov
Copy link
Contributor

atamazov commented Apr 16, 2021

I've already disabled all the tests I can and I didn't even enable conv3d for gfx908 as I recommended in the meeting.

In fact, I welcome any changes that improve the quality of testing. We just need to make such changes in separate PR. I will be glad if you begin (or even participate somehow in) this work. Thanks.

@atamazov atamazov merged commit 1161c17 into develop Apr 18, 2021
@atamazov
Copy link
Contributor

@ce1adon ㊗️

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.

6 participants