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

[Jay Foad via Phabricator via llvm-commits]

foad 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);
----------------
pravinjagtap wrote:
> 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 ?
> 
> 
Sounds fine but in this patch please just change the default for amdgpu-atomic-optimizations from false to true. The other option changes can be done in a separate patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152649