This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Allow frontends to disable null export for pixel shaders
ClosedPublic

Authored by critson on Jul 9 2021, 1:56 AM.

Details

Summary

Disable null export (for kills) when a frontend defines a pixel
shader as not exporting using amdgpu-color-export and
amdgpu-depth-export function attrbutes.
This allows the generation of export free pixel shaders.

Diff Detail

Event Timeline

critson created this revision.Jul 9 2021, 1:56 AM
critson requested review of this revision.Jul 9 2021, 1:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 9 2021, 1:56 AM
foad accepted this revision.Jul 9 2021, 2:30 AM

LGTM.

llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp
1353

Do these newly invented attributes need to be documented somewhere?

This revision is now accepted and ready to land.Jul 9 2021, 2:30 AM
critson added inline comments.Jul 9 2021, 3:32 AM
llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp
1353

I cannot find anywhere the existing ones like InitialPSInputAddr are documented?

foad added inline comments.Jul 9 2021, 3:42 AM
llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp
1353

OK then I guess you're off the hook :-/

ruiling added inline comments.Jul 11 2021, 6:22 PM
llvm/lib/Target/AMDGPU/SILateBranchLowering.cpp
70–82

Do you think we need to define a subtarget feature like AllowNoExportPS and check it here? The hardware behavior is different before and after gfx10, if we do not check this hardware difference, then this attributes will be gpu-generation-dependent (frontend should not set this attribute before gfx10). I am not sure whether this sounds a issue to others?

critson added inline comments.Jul 11 2021, 6:28 PM
llvm/lib/Target/AMDGPU/SILateBranchLowering.cpp
70–82

It depends, it seems we are going in the direction that it is up to frontends to ensure correct behaviour of exports. Rather than adding a subtarget feature we could just add an architecture check here.

ruiling added inline comments.Jul 11 2021, 9:57 PM
llvm/lib/Target/AMDGPU/SILateBranchLowering.cpp
70–82

Frontends are responsible for correctly inserting exp/exp_done to meet hardware requirement for most cases, there is only one exception AFAIK, the kill case which is what we are dealing with here. Architecture check sounds quite good to me. That would make the attribute just match the high level shader behavior.

critson updated this revision to Diff 357868.Jul 12 2021, 2:17 AM
  • Only allow no export PS on GFX10
critson updated this revision to Diff 358131.Jul 12 2021, 8:42 PM
  • Rename attributes to HasColorExport and HasDepthExport
critson edited the summary of this revision. (Show Details)Jul 12 2021, 8:42 PM
critson marked 4 inline comments as done.
arsenm requested changes to this revision.Jul 13 2021, 10:01 AM
arsenm added inline comments.
llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp
1355

Should stick to lowercase - separated naming convention with amdgpu- prefix

This revision now requires changes to proceed.Jul 13 2021, 10:01 AM
critson updated this revision to Diff 358501.Jul 13 2021, 8:28 PM
  • Rename function attributes to amdgpu-color-export and amdgpu-depth-export
critson edited the summary of this revision. (Show Details)Jul 13 2021, 8:30 PM
critson marked an inline comment as done.

The logic looks good to me now. Any concern from others?

foad accepted this revision.Jul 16 2021, 4:49 AM

LGTM.

llvm/lib/Target/AMDGPU/SILateBranchLowering.cpp
74

Maybe add a brief comment explaining what changed in the hardware in GFX10?

critson updated this revision to Diff 359520.Jul 16 2021, 7:37 PM
  • Add comments
critson marked an inline comment as done.Jul 16 2021, 7:39 PM

@arsenm are you happy for this to proceed now?

foad added inline comments.Jul 19 2021, 12:31 AM
llvm/lib/Target/AMDGPU/SILateBranchLowering.cpp
78

Typo "once" :)

critson updated this revision to Diff 359689.Jul 19 2021, 1:09 AM
  • Fix typo in comment
This revision was not accepted when it landed; it landed in state Needs Review.Jul 21 2021, 6:21 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.