This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU: Force sc0 and sc1 on stores for gfx940 and gfx941
ClosedPublic

Authored by kzhuravl on May 5 2023, 12:44 PM.

Diff Detail

Event Timeline

kzhuravl created this revision.May 5 2023, 12:44 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 5 2023, 12:44 PM
kzhuravl requested review of this revision.May 5 2023, 12:44 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 5 2023, 12:44 PM
kzhuravl added a reviewer: Restricted Project.May 5 2023, 12:48 PM
arsenm added a comment.May 9 2023, 4:39 AM

Should this be a feature set by default in the subtarget constructor instead? Should you be able to turn this off?

Should this be a feature set by default in the subtarget constructor instead? Should you be able to turn this off?

Users should not be able to turn this off. If user wants it off, user can use gfx942. Should it be moved to the subtarget constructor?

jmmartinez added inline comments.
llvm/lib/Target/AMDGPU/SIMemoryLegalizer.cpp
524

NIT: Is the use of the bitwise or " | " intended? I'd use the logical or " || " instead.

kzhuravl marked an inline comment as done.May 10 2023, 9:26 AM
kzhuravl added inline comments.
llvm/lib/Target/AMDGPU/SIMemoryLegalizer.cpp
524

It is intentional, we need both SC0 and SC1 bits set. If I switch this to || it will short circuit and not invoke enableSC1Bit.

I think that if this is a new property of the GFX940/941 targets, and turning it off shouldn't be possible, we shouldn't even bother with a feature and just set a bool in the ST for those targets

llvm/lib/Target/AMDGPU/SIMemoryLegalizer.cpp
524

IMHO then it needs a comment to explain that it's intentional, otherwise some innocent maintainer in the future could think it's a typo and change it

kzhuravl marked an inline comment as done.May 11 2023, 5:24 AM

I think that if this is a new property of the GFX940/941 targets, and turning it off shouldn't be possible, we shouldn't even bother with a feature and just set a bool in the ST for those targets

AFAIK, the subtarget only knows the generation and has access to features. The subtarget cannot differentiate between gfx940, gfx941, gfx942.

kzhuravl updated this revision to Diff 521322.May 11 2023, 8:32 AM
kzhuravl marked an inline comment as done.

Address review comments.

llvm/lib/Target/AMDGPU/SIMemoryLegalizer.cpp
524

Added comment.

scott.linder added inline comments.
llvm/lib/Target/AMDGPU/SIMemoryLegalizer.cpp
517–529

It may be a bit more verbose, but I would suggest just having a bool Changed variable, like other (admittedly more complicated) functions in the unit do. I don't think any future maintainer will mis-interpret this, even without the comment

arsenm added inline comments.May 12 2023, 6:54 AM
llvm/lib/TargetParser/TargetParser.cpp
330–332 ↗(On Diff #521322)

I don't see a reason to set this here. There's no need to expose this to the IR. We already leak too many target details here as it is

kzhuravl updated this revision to Diff 521643.May 12 2023, 7:13 AM
kzhuravl marked an inline comment as done.

Address review feedback

kzhuravl updated this revision to Diff 521645.May 12 2023, 7:16 AM
kzhuravl marked an inline comment as done.

Address @arsenm's comment

arsenm accepted this revision.May 12 2023, 7:20 AM
This revision is now accepted and ready to land.May 12 2023, 7:20 AM
This revision was landed with ongoing or failed builds.May 12 2023, 8:53 AM
This revision was automatically updated to reflect the committed changes.