declspec

[rocm-devops]

Original author: @awhittle

PR overview

For HipBLASLt integration and general library health. This PR can be read commit by commit (the ones adding the declspecs can probably be skimmed over).

This PR:

• Hides library symbols by default
• Adds the ROCROLLER_DECLSPEC macro
• Adds the ROCROLLER_DECLSPEC macro everywhere it is needed
• Fixes a unit test that messes with the settings and environment variables

Here's a primer:

• ROCROLLER_DECLSPEC allows library symbols to be visible
• When adding a new function, class, or struct, add ROCROLLER_DECLSPEC to the function declaration in the .hpp file if the following conditions are met:
  • The function, class, or struct should be used outside of the library, like in a test or an application
  • The function, class, or struct is is defined a .cpp file
    • DO NOT add it to inline functions. The user will have the function definition when including the .hpp.
• ROCROLLER_DECLSPEC should not appear in _impl.hpp or .cpp files
• You'll see LD errors if you didn't export the symbol correctly

Testing

Tests should pass as before.

NB: Run nm -CD librocroller.so to see the the symbols exposed.

Commit message

Add the ROCROLLER_DECLSPEC macro

Hides library symbols by default
Fix the settings unit 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

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	367
CodeQL	warning	rr/variantaccess-from-getelement Variant access from getElement; use graph.get<> instead.	lib/source/CodeGen/LoadStoreTileGenerator.cpp	371
CodeQL	warning	rr/variantaccess-from-getelement Variant access from getElement; use graph.get<> instead.	lib/source/KernelGraph/Transformations/FuseLoops.cpp	142
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

Error encountered generating coverage report

Skipped code cov for gfx942, no archived code_cov_gfx942.report found.

Artifacts

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

Commit Hashes

• 1abe066
• d24a9d7

[rocm-devops]

Original commenter: @awhittle

Is this getting added so many places because the functions are all being used in our tests? It seems like a lot more than would be needed for what our existing public API would need.

Short answer is yes, this is needed if we want to build our tests in Release mode.

Longer answer is that I think that building in Debug mode allows the symbols to be more accessible, and I think we could be more nuanced with exports in that case.

I opted not to go down the route of picking and choosing what to export right now because:

• We don't have a super stable API right now
• It would take a longer time for me to pick out everything that is being actively used, versus the find/replace approach I used
• CI scripts would need to change to only build and run tests in Debug mode

I agree that a more limited API is desirable. We could make a ticket to remove the unwanted declspecs, but... I think it might be better to do it now to avoid severe API thrash, if we have time now to do it.

[rocm-devops]

Original commenter: @nhenders

Original review comment on lib/include/rocRoller/Expression_impl.hpp at line 679

Why did these change from __attribute__((noinline)) to inline?

[rocm-devops]

Original commenter: @nhenders

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

I always find it helpful to leave some comments for nested #ifdef chains. e.g. #endif /* _WIN32 */.

[rocm-devops]

Original commenter: @awhittle

Original review comment on lib/include/rocRoller/Expression_impl.hpp at line 679

The definition for this templated function was located in a .cpp file, which means that the linker couldn't figure out the symbol with our new stricter compilation setup. I think the noinline was something to get this nearly working properly.

I've decided to move the templated function definition here to Expression_impl.hpp and inline it to solve this problem.

[rocm-devops]

Original commenter: @awhittle

Original review comment on lib/include/rocRoller/Expression_impl.hpp at line 679

Here's some documentation as to why this is an issue with templates.

[rocm-devops]

Original commenter: @awhittle

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

Yup, I can add those