[PATCH] D51969: [AMDGPU] Add an AMDGPU specific atomic optimizer.

[Neil Henning via Phabricator via llvm-commits]

sheredom added inline comments.


================
Comment at: lib/Target/AMDGPU/AMDGPUAtomicOptimizer.cpp:57
+
+  StringRef getPassName() const override { return "AMDGPU Atomic Optimizer"; }
+
----------------
arsenm wrote:
> This can be dropped
You mean I don't need to include a getPassName at all? I was just including it for consistency with the other AMDGPU level passes (a bunch at least have the method overridden).


================
Comment at: lib/Target/AMDGPU/AMDGPUAtomicOptimizer.cpp:207
+  //       \------------------> exit
+  BasicBlock *const EntryBB = I.getParent();
+  BasicBlock *const ExitBB = EntryBB->splitBasicBlock(&I);
----------------
arsenm wrote:
> All the consts are in the wrong place
I ran clang-format -i -style=file AMDGPUAtomicOptimizer.cpp and this is what it did!


================
Comment at: lib/Target/AMDGPU/AMDGPUAtomicOptimizer.cpp:243-244
+  Value *const ExtractHi = B.CreateExtractElement(BitCast, B.getInt32(1));
+  Function *const MbcntLoIntrinsic =
+      Intrinsic::getDeclaration(I.getModule(), Intrinsic::amdgcn_mbcnt_lo);
+  Function *const MbcntHiIntrinsic =
----------------
arsenm wrote:
> CreateIntrinsic
I can't use the IRBuilder's CreateIntrinsic because it will then try and mangle the amdgcn_mbcnt_lo like llvm.amdgcn.mbcnt.lo.i32, whereas we expect the name to be sans-type-suffix.


https://reviews.llvm.org/D51969