[PATCH] D74401: [MLIR] Add std.atomic_rmw op
River Riddle via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Feb 11 13:27:48 PST 2020
rriddle requested changes to this revision.
rriddle added inline comments.
This revision now requires changes to proceed.
================
Comment at: mlir/include/mlir/Dialect/StandardOps/Ops.td:227
+ buffer that the read and write will be performed against, as accessed by the
+ specified indicies. The arity of the indices is the rank of the memref.
+ The result represents the latest value that was stored.
----------------
typo: indicies -> indices
================
Comment at: mlir/include/mlir/Dialect/StandardOps/Ops.td:239
+
+ %cst = constant 1.0 : f32
+ %x = atomic_rmw %loaded = %I[%i] : memref<10xf32> {
----------------
Wrap these in a mlir code block
================
Comment at: mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp:2518
+template <typename OpType>
+Optional<SimpleAtomicMatch> matchAtomicBinaryOp(AtomicRMWOp atomicOp, OpType op,
+ LLVM::AtomicBinOp binOp) {
----------------
static functions should be in the global namespace and marked as `static`. Only classes should be placed within anonymous namespaces.
================
Comment at: mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp:2551
+ auto cmpOp = dyn_cast_or_null<CmpIOp>(op.condition().getDefiningOp());
+ if (!cmpOp) {
+ return llvm::None;
----------------
Drop trivial braces.
================
Comment at: mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp:2574
+ auto yieldOp = cast<AtomicRMWYieldOp>(terminator);
+ if (yieldOp.result().getParentRegion() != &atomicOp.body()) {
+ return SimpleAtomicMatch{LLVM::AtomicBinOp::xchg, yieldOp.result()};
----------------
Same here.
================
Comment at: mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp:2630
+
+// Wrap a llvm.cmpxchg operation in a while loop so that the operation can be
+// retried until it succeeds in atomically storing a new value into memory.
----------------
Top-level comments should be ///
================
Comment at: mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp:2688
+ auto init = rewriter.create<LLVM::LoadOp>(loc, dataPtr);
+ SmallVector<Value, 1> brProperOperands;
+ SmallVector<Block *, 1> brDestinations{loopBlock};
----------------
Don't create SmallVectors for things like this. Use ArrayRef or arrays.
================
Comment at: mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp:2716
+ SmallVector<Value, 1> condBrProperOperands{ok.res()};
+ SmallVector<Block *, 2> condBrDestinations{endBlock, loopBlock};
+ SmallVector<Value, 1> condBrRegionOperands{newLoaded.res()};
----------------
Same here.
================
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;
----------------
Remove trivial braces.
I would have expected that this could be covered by ODS constraints.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D74401/new/
https://reviews.llvm.org/D74401
More information about the llvm-commits
mailing list