[Backend] Only consider valid LoadOps for pipelining #3517
Conversation
| @@ -403,6 +407,17 @@ collectOpsToPipeline(scf::ForOp forOp, | |||
| // fixed. | |||
| if (!loadInfo.sharedEncoding) | |||
| continue; | |||
| } else if (isa<tt::LoadOp>(distAndUse.second)) { | |||
There was a problem hiding this comment.
I think there is still one problem left even after this fix:
We calculate stagesBetweenLoads based on the longest chain that is present in loadOpToDistAndUse. If we can "break the chain" after loadOpToDistAndUse is filled, this value may be incorrect in some cases.
This being said, this fix will help most cases, and does not really make the situation worse, we already can have improper loadOpToDistAndUse because we are dropping the loads here.
There was a problem hiding this comment.
I see. Thanks for the explanation, @pawelszczerbuk !
|
Thanks for the change @chenyang78 ! This will resolve some of the issues, but as I have mentioned in the inline comment, we still may hit some other problems in this code. I think we need to have single place where we drop the loads and can know the length of the "longest chain" of loads. Let's go with this change for now, and clean this code a bit soon. |
This PR fixed triton-lang#3454 Previously, we include all LoadOp(s) into opInfo, which content is used for pipelining. However, this approach may cause issues when we decide to not pipeline some LoadOps. Particularly, those LoadOps being excluded will end up with an inconsistent state which violates the relevant integraty checks, and hence cause ICEs. We fixed the issue by only including LoadOps that are valid for pipelining.
Head branch was pushed to by a user without write access
chenyang78
force-pushed
the
fix-pipelining
branch
from
3560c67 to
fcaed04
Compare
Make sense. Thanks for taking time reviewing the PR, @pawelszczerbuk! Seems the CI got stuck somehow. Do we need any approval to run the CI jobs? If so, could you help? Thanks! |
This PR fixes #3454
Previously, we included all LoadOp(s) into opInfo, whose contents are used for pipelining. However, this approach causes issues when we decide to not pipeline some LoadOps. Particularly, those LoadOps that are excluded will end up with an inconsistent state which violates the relevant integrity checks, and cause ICEs.
We fix the issue by only including LoadOps that are valid for pipelining.