[PATCH] D152649: [AMDGPU] Enable Atomic Optimizer and Default to Iterative Scan Strategy.

[Pravin Jagtap via Phabricator via llvm-commits]

pravinjagtap added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp:278-281
 static cl::opt<bool>
     EnableAtomicOptimizations("amdgpu-atomic-optimizations",
                               cl::desc("Enable atomic optimizations"),
+                              cl::init(true), cl::Hidden);
----------------
foad wrote:
> pravinjagtap wrote:
> > arsenm wrote:
> > > The old option can be dropped, none should be a value for the strategy 
> > > The old option can be dropped, none should be a value for the strategy 
> > 
> > I think, dropping the old option (`amdgpu-atomic-optimizations`) will break LLPC at the moment. 
> > 
> > I think right way to do this is, transfer the responsibility of enabling the pass to `amdgpu-atomic-optimizer-strategy` and keep the old option `amdgpu-atomic-optimizations` till LLPC removes all the usage of it.
> > 
> > @foad 
> Right, please keep it until LLPC has been updated again, then you can remove it.
> Right, please keep it until LLPC has been updated again, then you can remove it.

So, we will be having three options for strategy  { DPP, Iterative, None}. Here `None` will disable the pass (will be used in lit tests) & DPP/Iterative will enable the pass with given strategy.  

The `amdgpu-atomic-optimizer-strategy` will be defaulted to `Iterative` (not None as suggested by @arsenm ) and then we could drop the old option `amdgpu-atomic-optimizations`. 

Is this fine or is there any other way of handling this situation ?




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152649