[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) {
----------------
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


================
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;
----------------
flaub wrote:
> 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.
Hmmm, I thought there was one for this already. I think a lot of the current usages are abusing `mlir::getElementTypeOrSelf`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74401