[PATCH] D147408: [AMDGPU] Iterative scan implementation for atomic optimizer.

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUAtomicOptimizer.cpp:759
+  // Control flow is changed here after splitting I's block
+  assert(DTU.getDomTree().verify(DominatorTree::VerificationLevel::Fast));
+
----------------
Won't the regular post-pass verifier catch this?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUAtomicOptimizer.cpp:784-788
+    SmallVector<DominatorTree::UpdateType, 3> DTUpdates;
+    DTUpdates.push_back({DominatorTree::Insert, EntryBB, ComputeLoop});
+    DTUpdates.push_back({DominatorTree::Insert, ComputeLoop, ComputeEnd});
+    DTUpdates.push_back(
+        {DominatorTree::Delete, EntryBB, SingleLaneTerminator->getParent()});
----------------
Could use initializer list with all of these instead of push_back x 3. Also can use std::array


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUAtomicOptimizer.cpp:495
+
+  Type *const Ty = I.getType();
+  unsigned WaveFrontSize = ST->isWave32() ? 32 : 64;
----------------
arsenm wrote:
> east const throughout 
These weren't done, const is still weirdly placed


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp:277-285
+static cl::opt<bool> EnableAtomicOptimizationsUsingDPP(
+    "amdgpu-atomic-optimizer-use-dpp",
+    cl::desc("Use DPP in the atomic optimizer"), cl::init(true), cl::Hidden);
+
 // Enable atomic optimization
-static cl::opt<bool> EnableAtomicOptimizations(
-  "amdgpu-atomic-optimizations",
-  cl::desc("Enable atomic optimizations"),
-  cl::init(false),
-  cl::Hidden);
+static cl::opt<bool>
+    EnableAtomicOptimizations("amdgpu-atomic-optimizations",
----------------
arsenm wrote:
> I would expect this to be one cl::enum flag for the optimizer strategy. It would also still be better if this was a parsable pass parameter 
You shouldn't nede to come up with your own parsing with clEnumVal. For an example see the recent amdgpu-lower-module-lds-strategy


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147408