LWP100-1464: Fix fusing of tail loops

[rocm-devops]

Original author: @lwrubles

PR overview

For LWP100-1464.

Currently has a bunch of additional code for outputting the coordinate that a setCoordinate node is connected to, this will be removed from this PR and a new ticket created.

Testing

Currently mostly making use of existed jammed tests, additional tests are partially complete.

Commit message

Allow tail loops to be fused.

Previously fusing tail loops could not be done, so tail loops were not added for jammed tests as the unrolling of the X and Y loops caused multiple tail loops to exist. This PR fixes this issue, and also adds a new graph constraint that any given node cannot be in the body of two nodes which are not themselves nested in some order (i.e. if A is in the body of both B and C, either B is in the body of C or vice versa).

[rocm-devops]

Original commenter: @a1-mlselibci-npi

Generated Documentation

• Link to view Generated Docs.

[rocm-devops]

Original commenter: @a1-mlselibci-npi

CodeQL report

Results Summary

CodeQL
7

Full table of results

Tool	Severity	Code	Location	Line
CodeQL	warning	rr/variantaccess-from-getelement Variant access from getElement; use graph.get<> instead.	lib/source/CodeGen/LoadStoreTileGenerator.cpp	366
CodeQL	warning	rr/variantaccess-from-getelement Variant access from getElement; use graph.get<> instead.	lib/source/CodeGen/LoadStoreTileGenerator.cpp	370
CodeQL	warning	rr/variantaccess-from-getelement Variant access from getElement; use graph.get<> instead.	lib/source/KernelGraph/Transformations/FuseLoops.cpp	127
CodeQL	warning	rr/variantaccess-from-getelement Variant access from getElement; use graph.get<> instead.	test/unit/KernelGraphTest.cpp	1218
CodeQL	warning	rr/variantaccess-from-getelement Variant access from getElement; use graph.get<> instead.	test/unit/KernelGraphTest.cpp	1226
CodeQL	warning	rr/variantaccess-from-getelement Variant access from getElement; use graph.get<> instead.	test/unit/KernelGraphTest.cpp	1150
CodeQL	warning	rr/variantaccess-from-getelement Variant access from getElement; use graph.get<> instead.	test/unit/KernelGraphTest.cpp	1158

Links

• HTML
• Sarif (for download and usage in conjunction with SARIF viewers)

[rocm-devops]

Original commenter: @a1-mlselibci-npi

Code Coverage Report for gfx942

Summary

Type	Total	Missed	Master Missed	Missed Change	Coverage	Master Coverage	Coverage Change
Lines	46701	6451	6434	17	86.19%	86.17%	.02%
Functions	4589	685	682	3	85.07%	85.11%	-.04%
Regions	29210	6676	6667	9	77.14%	77.11%	.03%
Branches	16296	4194	4179	15	74.26%	74.23%	.03%

This PR adds/edits 29 newly uncovered lines.

Artifacts

• HTML Coverage Report and Diff
• File Coverage Summary
• Diff Text File
• Full Text Coverage Report
• Python Coverage Report

Commit Hashes

• 74718cf

[rocm-devops]

Original commenter: @maemmett

The performance numbers look bad. Can you investigate?

[rocm-devops]

Original commenter: @maemmett

Original review comment on test/unit/GEMMTest.cpp at line 3241

Just to confirm, we have a tail loop now, which has two extra ds_write_b64?

[rocm-devops]

Original commenter: @maemmett

Original review comment on lib/source/KernelGraph/Transformations/FuseLoops.cpp at line 301

                // Move operations that come after `nodeTag` to follow `fusedNodeTag`.
                for(auto const& child :

[rocm-devops]

Original commenter: @maemmett

Original review comment on lib/source/KernelGraph/Transformations/FuseLoops.cpp at line 311

                    // Make sure we don't introduce a cycle
                    std::unordered_set<int> toDelete;

[rocm-devops]

Original commenter: @maemmett

Original review comment on lib/source/KernelGraph/Transformations/FuseLoops.cpp at line 330

                // Fuse the bodies
                for(auto const& child :

[rocm-devops]

Original commenter: @maemmett

Original review comment on lib/source/KernelGraph/Transformations/FuseLoops.cpp at line 339

                // Make sure dependencies are satisfied
                for(auto const& parent :

[rocm-devops]

Original commenter: @lwrubles

Original review comment on test/unit/GEMMTest.cpp at line 3241

That is correct, yes!

[ammallya]

Merge Conflicts cannot import