[PATCH] D74401: [MLIR] Add std.atomic_rmw op

[River Riddle via Phabricator via llvm-commits]

rriddle added inline comments.


================
Comment at: mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp:2518
+template <typename OpType>
+Optional<SimpleAtomicMatch> matchAtomicBinaryOp(AtomicRMWOp atomicOp, OpType op,
+                                                LLVM::AtomicBinOp binOp) {
----------------
rriddle wrote:
> flaub wrote:
> > rriddle wrote:
> > > static functions should be in the global namespace and marked as `static`. Only classes should be placed within anonymous namespaces.
> > I'm OK with this, but I'm unfamiliar with this convention. What's the purpose behind having functions being `static` instead of being in the anonymous namespace? I was always under the impression that the two were functionally equivalent and that the more 'C++' way was to use anonymous namespaces.
> > 
> > Also, should I close out the namespace here and then add the static functions and then re-open the anonymous namespace? Or would it make sense to move these up above?
> It's about readability and consistency.
> 
> https://llvm.org/docs/CodingStandards.html#anonymous-namespaces
> Also, should I close out the namespace here and then add the static functions and then re-open the anonymous namespace? Or would it make sense to move these up above?

Whichever one makes sense. You can close the namespace or hoist the functions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74401