LWP100-1465 Address calculation test

[rocm-devops]

Original author: @yoonchoi

PR overview

For LWP100-1465

Replace #1122

This catch test obtains buffer instructions' base VGPR expressions(i.e. address calculation expression) and compare those original expressions with the widened to 64-bit expressions.

1. Comparisons of the pairs of (original expression, widened expression) are done within GPU kernels.
2. Currently, the comparison results are copied back to host memory.
   • Later, GPU assertion can be used.
3. Several GPU kernels are tested for sanity-check purposes including simple comparison of only one pair of original and promoted expressions.
   • The main test compares all pairs of expressions.
4. The main test runs over a set of configurations. A specific macro tile size 256, was avoided to ensure reasonable code-gen time. Other configurations can be added or removed as needed.
5. Related to PR #1157, which covers some of the changes in common GEMM graph relevant to the current PR. Similar change is also added to this current PR to facilitate testing (local or CI)

Testing

Ran catch/AddressCalculationTest

Commit message

Address calculation test

[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

recommendation
5

Full table of results

Type	Description	File Path	Start Line	Start Column	End Line	End Column
recommendation	Variant access from getElement; use graph.get<> instead.	/lib/source/KernelGraph/Transformations/AddF6LDSPadding.cpp	117	32	117.1	50
recommendation	Variant access from getElement; use graph.get<> instead.	/test/unit/KernelGraphTest.cpp	1052	17	1052	35
recommendation	Variant access from getElement; use graph.get<> instead.	/test/unit/KernelGraphTest.cpp	1060	17	1060	35
recommendation	Variant access from getElement; use graph.get<> instead.	/test/unit/KernelGraphTest.cpp	984	17	984	35
recommendation	Variant access from getElement; use graph.get<> instead.	/test/unit/KernelGraphTest.cpp	992	17	992	35

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	43694	6044	5437	607	86.17%	87.31%	-1.14%
Functions	4363	663	637	26	84.80%	85.31%	-.51%
Regions	23403	3797	5546	-1749	83.78%	78.71%	5.07%
Branches	13498	3435	3482	-47	74.55%	74.43%	.12%

This PR adds/edits 12810 newly uncovered lines.

Artifacts

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

Commit Hashes

• a5bfea9
• 3b86411

[rocm-devops]

Original commenter: @yoonchoi

Original review comment on lib/source/CodeGen/Arithmetic/BitwiseAnd.cpp at line 1

Will try to make a separate PR for this change as it can be independently tested.

[rocm-devops]

Original commenter: @squiring

Original review comment on lib/include/rocRoller/AssemblyKernel_impl.hpp at line 233

addCommandArguments() could probably be modified to have this change.

[rocm-devops]

Original commenter: @squiring

Original review comment on lib/include/rocRoller/CommandSolution.hpp at line 207

This needs a better name. I assumed this would apply the CleanArguments graph transformation.

[rocm-devops]

Original commenter: @squiring

Original review comment on lib/source/CommandSolution.cpp at line 132

Most of these log statements should be removed once this is all working.

[rocm-devops]

Original commenter: @squiring

Original review comment on test/catch/AddressCalculationTest.cpp at line 207

Have a look at how the TernaryExpressionTest is structured. As much as possible of the individual test code should be in the test, not in separate functions.

[rocm-devops]

Original commenter: @yoonchoi

Original review comment on lib/include/rocRoller/AssemblyKernel_impl.hpp at line 233

Did you mean replacing the current implementation of addCommandArguments() (it is located right above) with this new implementation and removing addNewCommandArguments()? The difference is at line 209.
Without the check, an assertion failure will occur in case some arguments are already in the list. This function is used in lowerToKernelArguments(), which is called after the original kernel's preamble is scheduled. I noticed some of the command arguments are already lowered down to kernel's argument, but not all of them. At this time cleanArgument pass is already called over the original kernel graph, as the graph is obtained right before generateKernelSource(). Maybe lowerToKernelArguments() should be called before kernel's preamble is scheduled and then no command argument has lowered down to kernel arguments?

[rocm-devops]

Original commenter: @yoonchoi

Original review comment on lib/include/rocRoller/CommandSolution.hpp at line 207

If this function still needs to exists, I will look for more appropriate name.

[rocm-devops]

Original commenter: @yoonchoi

Original review comment on lib/source/CommandSolution.cpp at line 132

Agreed. Will remove at the end of the commits. For now, I still need them.

[rocm-devops]

Original commenter: @yoonchoi

Original review comment on test/catch/AddressCalculationTest.cpp at line 207

Does the newly uploaded change look closer to what was meant? Please see test_equal() into which I move the kernel body. If that is more desirable, I can do so for other kernel bodies.

At the end of this file TEST_CASEs have been changed to

1. cover multiple sizes / types. (Done only for test_equal(). Other tests are mainly simpler sanity check, which I don't think requires multiple sizes)
2. Each TEST_CASE now has a unique name.

[rocm-devops]

Original commenter: @yoonchoi

Original review comment on test/common/common/CommonGraphs_impl.hpp at line None

The code should be removed.

[rocm-devops]

Original commenter: @yoonchoi

Original review comment on test/common/common/TestValues.hpp at line None

New: remove

[rocm-devops]

Original commenter: @yoonchoi

Original review comment on lib/source/ExpressionTransformations/FastDivision.cpp at line 1

Remove changes in this file

[rocm-devops]

Original commenter: @yoonchoi

Original review comment on test/catch/AddressCalculationTest.cpp at line None

Add more specific numbers of macM, macN etc.