This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Add target features for GDS and GWS
ClosedPublic

Authored by foad on Jul 31 2023, 9:39 AM.

Details

Reviewers
b-sumner
arsenm
Group Reviewers
Restricted Project
Commits
rGc2093b85044d: [AMDGPU] Add target features for GDS and GWS
Summary

GFX9 subtargets from GFX90A onwards lack GDS but still have GWS.

Diff Detail

Event Timeline

foad created this revision.Jul 31 2023, 9:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 31 2023, 9:39 AM
foad requested review of this revision.Jul 31 2023, 9:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 31 2023, 9:39 AM
foad added a comment.Jul 31 2023, 9:40 AM

Do we want a separate GWS feature? Or a combined "hasGDSAndGWS" feature?

Do we want a separate GWS feature? Or a combined "hasGDSAndGWS" feature?

We do need a feature that include GWS. GDS was dropped before GWS in GFX9, but compute never used GDS. So GDSandGWS would probably work.

Do we want a separate GWS feature? Or a combined "hasGDSAndGWS" feature?

We do need a feature that include GWS. GDS was dropped before GWS in GFX9, but compute never used GDS. So GDSandGWS would probably work.

I didn't realize GDS was dropped in gfx9, should split the two then

Do we want a separate GWS feature? Or a combined "hasGDSAndGWS" feature?

We do need a feature that include GWS. GDS was dropped before GWS in GFX9, but compute never used GDS. So GDSandGWS would probably work.

I didn't realize GDS was dropped in gfx9, should split the two then

Sorry for mangling this. GDS was dropped in gfx90a. GWS was not dropped in gfx9.

foad updated this revision to Diff 545987.Aug 1 2023, 3:05 AM

Separate features for GDS and GWS.

foad retitled this revision from [AMDGPU][RFC] Add a target feature for GDS to [AMDGPU] Add target features for GDS and GWS.Aug 1 2023, 3:06 AM
foad edited the summary of this revision. (Show Details)
foad added a comment.Aug 1 2023, 3:08 AM

Do we want a separate GWS feature? Or a combined "hasGDSAndGWS" feature?

We do need a feature that include GWS. GDS was dropped before GWS in GFX9, but compute never used GDS. So GDSandGWS would probably work.

I didn't realize GDS was dropped in gfx9, should split the two then

Sorry for mangling this. GDS was dropped in gfx90a. GWS was not dropped in gfx9.

What about gfx909 and gfx90c?

In the current patch I have assumed gfx908 and gfx909 both have GDS, and gfx90a and gfx90c (and gfx94x) lack it.

AlexVlx added a subscriber: AlexVlx.Aug 1 2023, 3:32 AM

Do we want a separate GWS feature? Or a combined "hasGDSAndGWS" feature?

We do need a feature that include GWS. GDS was dropped before GWS in GFX9, but compute never used GDS. So GDSandGWS would probably work.

I didn't realize GDS was dropped in gfx9, should split the two then

Sorry for mangling this. GDS was dropped in gfx90a. GWS was not dropped in gfx9.

What about gfx909 and gfx90c?

In the current patch I have assumed gfx908 and gfx909 both have GDS, and gfx90a and gfx90c (and gfx94x) lack it.

Somewhat confusingly, gfx90c is an older APU (Renoir IIRC), and not a successor to gfx90a, which would still have GDS, I believe.

foad updated this revision to Diff 545995.Aug 1 2023, 3:49 AM

Restore GDS to gfx90c.

foad added a comment.Aug 1 2023, 3:49 AM

Do we want a separate GWS feature? Or a combined "hasGDSAndGWS" feature?

We do need a feature that include GWS. GDS was dropped before GWS in GFX9, but compute never used GDS. So GDSandGWS would probably work.

I didn't realize GDS was dropped in gfx9, should split the two then

Sorry for mangling this. GDS was dropped in gfx90a. GWS was not dropped in gfx9.

What about gfx909 and gfx90c?

In the current patch I have assumed gfx908 and gfx909 both have GDS, and gfx90a and gfx90c (and gfx94x) lack it.

Somewhat confusingly, gfx90c is an older APU (Renoir IIRC), and not a successor to gfx90a, which would still have GDS, I believe.

OK, so only gfx90a and gfx94x lack GDS.

arsenm accepted this revision.Aug 1 2023, 2:37 PM
arsenm added inline comments.
llvm/test/CodeGen/AMDGPU/gds-unsupported.ll
2

don't need -verify-machineinstrs

This revision is now accepted and ready to land.Aug 1 2023, 2:37 PM
This revision was automatically updated to reflect the committed changes.