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

[Jay Foad via Phabricator via llvm-commits]

foad added a comment.

Code LGTM. I'm not sure how far we want to go with disabling this optimization in lit tests that are not specifically testing the optimization itself.



================
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:
> 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.


================
Comment at: llvm/test/CodeGen/AMDGPU/GlobalISel/mubuf-global.ll:2
 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc -global-isel -mtriple=amdgcn-mesa-mesa3d -mcpu=tahiti < %s | FileCheck -check-prefix=GFX6 %s
 ; RUN: llc -global-isel -mtriple=amdgcn-mesa-mesa3d -mcpu=hawaii < %s | FileCheck -check-prefix=GFX7 %s
----------------
_Maybe_ disable atomic optimization in this test, since it obscures what we are testing for?


================
Comment at: llvm/test/CodeGen/AMDGPU/dag-divergence-atomic.ll:2
 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc -march=amdgcn -mcpu=gfx90a -o - %s | FileCheck %s
 
----------------
_Maybe_ disable atomic optimization in this test, since it obscures what we are testing for?


================
Comment at: llvm/test/CodeGen/AMDGPU/gds-allocation.ll:2
 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc -mtriple=amdgcn-amd-amdpal -mcpu=gfx900 -verify-machineinstrs < %s | FileCheck -check-prefix=GCN %s
 
----------------
_Maybe_ disable atomic optimization in this test, since it obscures what we are testing for?


================
Comment at: llvm/test/CodeGen/AMDGPU/llc-pipeline.ll:260
 ; GCN-O1-NEXT:        Post-Dominator Tree Construction
+; GCN-O1-NEXT:        Cycle Info Analysis
+; GCN-O1-NEXT:        Uniformity Analysis
----------------
This is a bit unfortunate. Is there anything we can do about it? Does cycle info even have an API for updating it?


================
Comment at: llvm/test/CodeGen/AMDGPU/should-not-hoist-set-inactive.ll:2
 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 2
 ; RUN: llc -march=amdgcn -mcpu=gfx1010 -verify-machineinstrs < %s | FileCheck %s -check-prefix=GCN
 
----------------
_Maybe_ disable atomic optimization in this test, since it obscures what we are testing for?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152649