[PATCH] D148199: [AMDGPU] Add a new command line argument amdgpu-atomic-optimizer-use-dpp

[Jay Foad via Phabricator via llvm-commits]

foad added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp:1121-1122
 
-  if (isPassEnabled(EnableAtomicOptimizations, CodeGenOpt::Less)) {
+  if (isPassEnabled(EnableAtomicOptimizations, CodeGenOpt::Less) ||
+      isPassEnabled(EnableAtomicOptimizationsUsingDPP, CodeGenOpt::Less)) {
     addPass(createAMDGPUAtomicOptimizerPass());
----------------
arsenm wrote:
> pravinjagtap wrote:
> > pravinjagtap wrote:
> > > pravinjagtap wrote:
> > > > foad wrote:
> > > > > arsenm wrote:
> > > > > > 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.
> > > > > > 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.
> > > > > 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.
> > > > 
> > > > 
> > > > 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 
> > > 
> > > 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. 
> > 
> > 
> 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
> 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`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148199/new/

https://reviews.llvm.org/D148199