[PATCH] D98650: [NVPTX] Enable lowering of atomics on local memory

[Artem Belevich via Phabricator via llvm-commits]

tra accepted this revision.
tra added a comment.
This revision is now accepted and ready to land.

Few nits. LGTM overall.



================
Comment at: llvm/include/llvm/Transforms/Scalar/LowerAtomic.h:17
 
+#include "llvm/IR/Instructions.h"
 #include "llvm/IR/PassManager.h"
----------------
We could probably get by with a `class AtomicRMWInst;`. We don't need the whole header just for the pointer type.


================
Comment at: llvm/include/llvm/Transforms/Scalar/LowerAtomic.h:32
+/// succeeds.
+bool LowerAtomicRMWInst(AtomicRMWInst *RMWI);
 }
----------------
Nit: function names should be `camel case, and start with a lower case letter (e.g. openFile() or isFoo()).`, according to LLVM style guide: https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly


================
Comment at: llvm/lib/Target/NVPTX/NVPTXAtomicLower.cpp:64-66
+namespace llvm {
+void initializeNVPTXAtomicLowerPass(PassRegistry &);
+}
----------------
`llvm::initializeNVPTXAtomicLowerPass(PassRegistry &);` would do.



================
Comment at: llvm/test/CodeGen/NVPTX/atomic-lower.ll:7-12
+  %res = atomicrmw fadd double addrspace(5)* %ptr, double %val monotonic, align 8
+  ret double %res
+; CHECK:   %1 = load double, double addrspace(5)* %ptr, align 8
+; CHECK-NEXT:   %2 = fadd double %1, %val
+; CHECK-NEXT:   store double %2, double addrspace(5)* %ptr, align 8
+; CHECK-NEXT:   ret double %1
----------------
The file should be renamed to `atomic-lower-local.ll` and there should be a comment describing that the test ensures that we're able to lower atomics in local AS in principle and that in case of NVPTX we happen to lower them to regular load/stores. Without the explanation, the CHECK lines by themselves are somewhat puzzling as there is nothing about atomics in the IR they check for.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98650