Skip to content

Stream-k initial support - #1035

Merged
AlexBrownAMD merged 46 commits into
ROCm:developfrom
AlexBrownAMD:StreamKMerge2
Sep 7, 2024
Merged

AlexBrownAMD merged 46 commits into
ROCm:developfrom
AlexBrownAMD:StreamKMerge2

Conversation

@AlexBrownAMD

Copy link
Copy Markdown
Contributor

This PR introduces initial stream-k support into hipBLASLt. SGEMM, HGEMM, and BGEMM have been validated with a large number of parameter combinations and problem sizes, and this PR includes some new CI tests for these problem types. The stream-k algorithm helps deliver improved and stable performance with smaller library size. For full details, see the presentations or paper.

Most of the code changes for stream-k are isolated in a component class to help with code readability. Since stream-k is not compatible with GSU, all the GSU related code has also been moved from the KernelWriter classes into it's own component class, and an additional setting has been added to allow generating kernels with GSU disabled (GSU=0 disables it, GSU=1 is still the default, a new CI test for both settings is included). A component for persistent looping has also been added since stream-k operates on a loop. Descriptions of how to use the stream-k options and run-time environment variables are included in the comments in Common.py.

Full stream-k support in hipBLASLt is still a work in progress, and this PR is meant to introduce the initial core functionality. There are a number of known issues that I will work in in future PRs and there are tickets to track, including:

  • Support and validation for all data types
  • Consolidate some of the fixup and partials write procedures with the general global write module
  • Compare and consolidate the stream-k XCC mapping feature with the WorkGroupMappingXCC option
  • Group gemm support
  • Additional plans for code improvement and porting of more stream-k features from Tensile repo are described in tickets

AlexBrownAMD and others added 30 commits June 4, 2024 20:00
Modularize GSU code so it is more contained, reduces branch points in KernelWriter code.
Fix a temp register allocation bug.
Fix alignment of some registers rather than rely on a specific order.
#3)

* Support for predicate arithmetic intensity added on top of streamK branch in Alex's repo
* Analytical grid size prediction model for Stream-K
Porting code gen changes for stream-k

@nakajee nakajee left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

First round review. I will spend more time later...

##############################################################################
class StoreConstConfig:
def __init__(self, kernelWriter, kernel, ss, gwvw, edge, beta, atomic):
def __init__(self, kernelWriter, kernel, ss, gwvw, edge, beta, atomic, isWorkspace=False):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Don't we need to change halfDataRegPerVI (line 105).
It refers DestDataType.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think line 105 is OK. It is just checking if the write target is a half type (and excluding 8-bit types). The isWorkspace parameter makes bpeC use internal compute size instead of external write size so numVgprsPerDataPerVI is correct for the partials write case.

Comment thread tensilelite/Tensile/SolutionStructs.py Resolved
Comment thread tensilelite/Tensile/SolutionStructs.py Resolved
@@ -0,0 +1,58 @@

GlobalParameters:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we need skip here.
This should be only for gfx9xx

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch, just posted a fix for this this test as well as the other stream-k tests that were added

Comment thread tensilelite/Tensile/SolutionStructs.py Resolved
Comment thread tensilelite/Tensile/SolutionStructs.py Resolved
@nakajee nakajee added the gfx94x Run CI on gfx94x label Aug 22, 2024
@eidenyoshida eidenyoshida added the noCI Disable testing on supported CI systems: math libraries CI has this feature enabled.. label Aug 22, 2024
* `(n + d - 1)` would overflow.
*/
template <typename N, typename D>
__device__ __host__ inline constexpr N safe_ceil_div(N n, D d)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

neoblizz
neoblizz previously approved these changes Aug 23, 2024
@nakajee

nakajee commented Aug 27, 2024 •

Copy link
Copy Markdown
Contributor

I tried HHS TN + StreamK: [0,1,2,3] but I see some issues.
(1) warnings
TensileInstructions::WARNING: RegisterPool::remove(0,95) pool0 still in use
TensileInstructions::WARNING: RegisterPool::remove(0,95) pool1 still in use
TensileInstructions::WARNING: RegisterPool::add(0,149) pool0 already available
TensileInstructions::WARNING: RegisterPool::add(0,149) pool1 already available
TensileInstructions::WARNING: RegisterPool::remove(0,95) pool0 still in use
TensileInstructions::WARNING: RegisterPool::remove(0,95) pool1 still in use
TensileInstructions::WARNING: RegisterPool::add(0,149) pool0 already available
TensileInstructions::WARNING: RegisterPool::add(0,149) pool1 already available
TensileInstructions::WARNING: RegisterPool::remove(0,95) pool0 still in use
TensileInstructions::WARNING: RegisterPool::remove(0,95) pool1 still in use
TensileInstructions::WARNING: RegisterPool::add(0,149) pool0 already available
TensileInstructions::WARNING: RegisterPool::add(0,149) pool1 already available

(2) error
File "tensilelite/Tensile/Components/StreamK.py", line 1792, in storeBranches
module.add(self.storeBranchesCommon(writer, kernel, skPartialsLabel, vectorWidths, element, tmpVgpr, cvtVgprStruct))
NameError: name 'element' is not defined

This is the folkparameters I tried.
- 1LDSBuffer: [1]
- DepthU: [64]
- MatrixInstruction:
- [16, 16, 16, 1, 1, 4, 8, 4, 1] # MT 256x128
- PrefetchGlobalRead: [2]
- PrefetchLocalRead: [1]
- SourceSwap: [1]
- StaggerU: [0]
- StreamK: [0,1,2,3]

@nakajee

nakajee commented Aug 27, 2024

Copy link
Copy Markdown
Contributor

Please make sure all CI tests pass on your local node

  • tensilelite tox
  • hipblaslt-test

@AlexBrownAMD

Copy link
Copy Markdown
Contributor Author

Thanks for the notes. I fixed issue #2 as well as a bug in the xccmap. Still working on debugging issue #1.

module.add(VReadfirstlaneB32(dst=sgpr(tmpSgpr+2), src=vgpr("Serial"), comment="Wave 0 updates flags"))
module.add(SCmpEQU32(src0=sgpr(tmpSgpr+2), src1=0, comment="Check for wave 0"))
module.add(SCBranchSCC0(labelName=skipFlagReset.getLabelName(), comment="Skip flag reset"))
module.add(SMovB32(dst=sgpr(tmpSgpr+2), src=0, comment="flag data"))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Small optimization.
tmpSgpr+2 is already 0 here based on the above condition.
We can remove SMovB32.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch, just posted an update

skipFlagReset = Label(label=writer.labels.getNameInc("SK_SkipFlagReset"), comment="")
module.add(VReadfirstlaneB32(dst=sgpr(tmpSgpr+2), src=vgpr("Serial"), comment="Wave 0 updates flags"))
module.add(SCmpEQU32(src0=sgpr(tmpSgpr+2), src1=0, comment="Check for wave 0"))
module.add(SCBranchSCC0(labelName=skipFlagReset.getLabelName(), comment="Skip flag reset"))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Quick question.
Why we need this code only for tensilelite?
I did not see it in Tensile.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In Tensile the synchronization flags are stored in the workspace, which is supplied by the user and could contain any values, so there is a pre-kernel that initializes the flags to 0.
In TensileLite the flags are stored in the Synchronizer buffer which is kept in the handle and initialized to 0. In this case we also have to reset the flags to 0 after it they have been read so that they're ready for the next kernel to run. The first version on this PR had all 4 wavefronts in a workgroup doing the s_store to the flag, which was creating a race condition between the WG doing the partial tile setting it to 1 and the WG doing the fixup resetting it to 0. The kernel would sometimes end with some flags set to 1 instead of 0 causing kernels that run afterwards to fail.
This update changes it so only wavefront 0 updates the flag value (and wavefronts in a WG are still synchronized with a barrier to ensure all partial data is fully written before setting the flag)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I just added one more test file based on the test that most easily reproduced the race condition

@nakajee

nakajee commented Sep 6, 2024

Copy link
Copy Markdown
Contributor

Thanks for your update.
Since "noCI" label is added, please run all CI tests (tensilelite tox + hipblaslt-test) on your local node and paste the result here.

std::shared_ptr<hipDeviceProp_t> deviceProp;

auto adapter = get_library_and_adapter(&library, &deviceProp, handle->device);
<<<<<<< HEAD

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Merge issue.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks, got it fixed

std::shared_ptr<hipDeviceProp_t> deviceProp;

auto adapter = get_library_and_adapter(&library, &deviceProp, handle->device);
<<<<<<< HEAD

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Merge issue.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@AlexBrownAMD

Copy link
Copy Markdown
Contributor Author

Finished running tensilelite tox tests, working on hipblaslt-test

=================================================================================== 75 passed, 31 skipped, 44 warnings in 7744.02s (2:09:04) ====================================================================================
py310: exit 0 (7744.30 seconds) /home/stebrown/hipBLASLt2/tensilelite> py.test -v --basetemp=/home/stebrown/hipBLASLt2/tensilelite/.tox/py310/tmp --junit-xml=/home/stebrown/hipBLASLt2/tensilelite/python_tests.xml --junit-prefix=py310 --color=yes -n 4 --prebuilt-client=/home/stebrown/hipBLASLt2/tensilelite/.tox/py310/client/0_Build/client/tensile_client ./Tensile/Tests/ -m common pid=1141334
py310: OK (7888.08=setup[14.73]+cmd[1.80,127.24,7744.30] seconds)
congratulations :) (7888.16 seconds)

@nakajee nakajee left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for your update.
Looks good.

@AlexBrownAMD

Copy link
Copy Markdown
Contributor Author

All hipblaslt-test test cases passing as well:

[----------] Global test environment tear-down
[==========] 48206 tests from 13 test suites ran. (2278421 ms total)
[ PASSED ] 48206 tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gfx94x Run CI on gfx94x noCI Disable testing on supported CI systems: math libraries CI has this feature enabled..

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants