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

[Frank Laub via Phabricator via llvm-commits]

flaub marked 3 inline comments as not done.
flaub 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:
> 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?


================
Comment at: mlir/lib/Dialect/StandardOps/Ops.cpp:3019
+  Type elementType = parentOp.getMemRefType().getElementType();
+  if (elementType != op.result().getType()) {
+    return op.emitOpError() << "needs to have type " << elementType;
----------------
rriddle wrote:
> Remove trivial braces.
> 
> I would have expected that this could be covered by ODS constraints.
Did you have a specific trait in mind? I'm comparing the parent's element type to the op's operand type. I didn't see anything in `OpBase.td` at first glance.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74401