This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Add a new command line argument amdgpu-atomic-optimizer-use-dpp
AbandonedPublic

Authored by pravinjagtap on Apr 13 2023, 12:18 AM.

Details

Summary

The current implementation of atomic optimization uses DPP with WWM for
scan computations. This new command line argument has exactly the same
behavior as old amdgpu-atomic-optimizations.

The new approach proposed in D147408, required to have two different
code paths for DPP and iterative scan. The implementation will be
selected based on this command line argument.

This will allow the the graphics pipeline to set the new argument
instead of the old one.

Diff Detail

Event Timeline

pravinjagtap created this revision.Apr 13 2023, 12:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 13 2023, 12:18 AM
pravinjagtap requested review of this revision.Apr 13 2023, 12:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 13 2023, 12:18 AM
foad added inline comments.Apr 13 2023, 1:56 AM
llvm/test/CodeGen/AMDGPU/atomic_load_add.ll
1 ↗(On Diff #513083)

I guess these tests really want to disable the atomic optimizer pass, to avoid widespread changes to the generated code. If so, they should continue using -amdgpu-atomic-optimizations and we should continue to support that option with its current meaning.

pravinjagtap added inline comments.Apr 13 2023, 2:26 AM
llvm/test/CodeGen/AMDGPU/atomic_load_add.ll
1 ↗(On Diff #513083)

If I understand correctly, the tests which uses -amdgpu-atomic-optimizations=*true* can be updated with a new option? whereas the tests which uses -amdgpu-atomic-optimizations=*false* should continue to support old option?

I thought we will be deprecating -amdgpu-atomic-optimizations option completely and Atomic Optimizer Pass will depend on only a new option -amdgpu-atomic-optimizer-use-dpp.

foad added inline comments.Apr 13 2023, 2:38 AM
llvm/test/CodeGen/AMDGPU/atomic_load_add.ll
1 ↗(On Diff #513083)

If I understand correctly, the tests which uses -amdgpu-atomic-optimizations=*true* can be updated with a new option? whereas the tests which uses -amdgpu-atomic-optimizations=*false* should continue to support old option?

Yes that makes sense.

I thought we will be deprecating -amdgpu-atomic-optimizations option completely

That was my first suggestion, but if we have lots of tests that use it then maybe we should keep it just for the tests? I don't feel too strongly about it.

Rebased and addressed review comment about test flag

pravinjagtap edited the summary of this revision. (Show Details)Apr 13 2023, 3:48 AM
arsenm added inline comments.Apr 14 2023, 2:53 PM
llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
1121–1122

Two flags to enable the same pass is bad, and this doesn't change the pass behavior. The behavior change could be a pass parameter.

foad added inline comments.Apr 16 2023, 11:22 PM
llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
1121–1122

Two flags to enable the same pass is bad

That's intentional and temporary. It's to avoid a flag-day change for downstream users like LLPC when they switch over to the new option.

pravinjagtap added inline comments.Apr 16 2023, 11:54 PM
llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
1121–1122

Two flags to enable the same pass is bad, and this doesn't change the pass behavior. The behavior change could be a pass parameter.

1121–1122

The behavior change could be a pass parameter.

If I understand correctly you are suggesting to have string here instead of boolean true or false value as parameter? Something like:

-amdgpu-atomic-optimizer-scan=dpp or -amdgpu-atomic-optimizer-scan=iterative and based on the parameter value of this new command line we will be taking different code paths

pravinjagtap marked 2 inline comments as not done.Apr 17 2023, 12:15 AM
pravinjagtap added inline comments.Apr 19 2023, 2:10 AM
llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
1121–1122

and this doesn't change the pass behavior

Keeping the pass behavior same was also intentional with new command line argument. This will buy some time for users to adapt to a new command line argument.

arsenm added inline comments.Apr 19 2023, 6:34 AM
llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
1121–1122

No, I mean move away from command line arguments entirely in favor of the new pass managers pass manager options (e.g. https://github.com/llvm/llvm-project/blob/139f678c7849d90eff715da99d80bbb8b75a79a5/llvm/lib/Transforms/Scalar/SROA.cpp#L5159)

Adding a second flag which simply controls adding the pass to the pipeline as is done here is pointless

foad added inline comments.Apr 19 2023, 12:20 PM
llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
1121–1122

No, I mean move away from command line arguments entirely in favor of the new pass managers pass manager options

What would a frontend like LLPC have to do to set such an option? Currently it calls LLVMTargetMachine::addPassesToEmitFile which calls TargetPassConfig::addISelPasses which calls into GCNPassConfig to create a bunch of passes including AMDGPUAtomicOptimizerPass.

arsenm requested changes to this revision.Jun 27 2023, 3:15 PM

I think this is obsolete now

This revision now requires changes to proceed.Jun 27 2023, 3:15 PM
pravinjagtap abandoned this revision.Jun 28 2023, 1:42 AM