[MLIR] LLVM dialect: Add llvm.atomicrmw

[flaub]

Summary

This op is the counterpart to LLVM's atomicrmw instruction. Note that
volatile and syncscope attributes are not yet supported.

This will be useful for upcoming parallel versions of affine.for and generally
for reduction-like semantics.

[jfb]

mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td

751

Could you call this one relaxed, so it matches C++, instead of the LLVM IR name?

[merge_guards_bot]

Unit tests: pass. 61744 tests passed, 0 failed and 780 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[flaub]

mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td

751

I don't have a strong intuition on this, but it seems like it should match LLVM IR faithfully since the LLVM dialect in MLIR should basically mirror it. Also, the context for this change is to support other MLIR dialect lowerings (such as the Affine dialect), and thus don't have any relationship to C++ (except perhaps distantly).

I'd suggest that we call it relaxed in a C++ dialect (or some other frontend), which would lower to this one.

[jfb]

mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td

751

acquire, release, ace_rel, and seq_cst came from C++ (and then C). @jyasskin might remember why monotonic wasn't called relaxed... but IMO it would be best to match the C++ naming for all these orderings, not all but one.

[flaub]

mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td

751

I see, I took the namings from: https://llvm.org/docs/LangRef.html#ordering

But if that's out of date, I can update it.

Ideally, this whole dialect would be mechanically generated from LLVM IR sources/td, so whichever name matches the existing IR is probably what we should use.

[flaub]

mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td

751

I think we should stay in sync with: https://github.com/llvm/llvm-project/blob/master/llvm/include/llvm/Support/AtomicOrdering.h#L81

[ftynse]

Thanks! Most of my comments are minor.

Please also add a translation test to test/Target/llvmir.mlir.

mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td

729

This is unfortunate that this is conflicting with C++ keywords. I differentiated between enum case names (capitalized and thus non-conflicting) and the printed syntax for linkage attributes. That required boilerplate switches in parsing and printing though. I'm fine with these names, but we should consider a general solution for this problem eventually.

751

I am quite strongly opposed to diverging from LLVM names here since the LLVM dialect is modeling LLVM IR, not C++. If we change the name in LLVM IR, we can do the same in the dialect. I have a vague preference for reusing enum names rather than IR keywords.

The documentation in LLVM seems in sync with the code.

Ideally, this whole dialect would be mechanically generated from LLVM IR sources/td, so whichever name matches the existing IR is probably what we should use.

Yes, except that LLVM does not have a .td for instructions, only intrinsics. I am (lazily) thinking in the direction of having a single source of truth for instructions.

766

Nit: the name op is unfortunate as it is often used as short for the current operation.

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp

1377

We tend to use single quotes for single characters (there's an overload on operator<< in some cases).

1379

Dereferencing should not be necessary anymore since mlir::Value is now a value-type.

1433

You can declare op and ordering as StringAttr directly, OpAsmParser does the check for you then.

1436

It would be unclear to the user what 'ordering' means because this word does not appear in the syntax. This code should disappear anyway if you use the typed parser function.

1448

Nit: just write here directly instead of declaring a new vector of attributes?

1454

Nit: return op == AtomicBinOp::fadd || op == AtomicBinOp::fsub; looks 7 times shorter

1464

Auto-generated verifier runs before this and it already checked that all types are LLVMType. Just use cast here and drop the null checks below.

1466

I'd prefer identifying operands using their position. It may sound counter-intuitive, but it is never clear from the IR itself or the error message which operand relates to each name. And counting is easier and less context-switching then opening the doc to check the names, and then still counting in the IR to fix the right operand.

1475–1479

Please add the check that arguments are power-of-two integers otherwise (https://llvm.org/docs/LangRef.html#i-atomicrmw), except for xchg that accepts both floats and integers.

mlir/test/Dialect/LLVMIR/roundtrip.mlir

224–225

I'd rather just pass the pointer in the function to make sure we only check the operation we care about here (atomicrmw)

226

The {volatile} part does not seem necessary for the test.

226

Please avoid using SSA names in the CHECK strings, there are no guarantees they are preserved. Use %{{.*}} instead or capture the names with %[[name-in-tablegen:.*]], see https://mlir.llvm.org/getting_started/TestingGuide/ for more detail.

[flaub]

Thanks for the review :)

I'll try to follow up tomorrow and also look at writing a translation test (I wasn't aware of it, thanks for the heads up).

mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td

766

Is bin_op reasonable?

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp

1379

Oops, good catch.

1433

Good to know, I'll do that in my next round.

I was attempting to follow other code in this file that looked like it was doing a similar thing.

1448

I don't understand what this comment means.

Actually I was attempting to follow the other custom parser impls in this file.

1454

Except it makes it much harder to maintain. This style, while longer, allows one to easily add a new case when a change in the IR arrives, and makes diffs much easier to comprehend too.

On a minor note, this style matches the exact same function in the LLVM IR:
https://github.com/llvm/llvm-project/blob/master/llvm/include/llvm/IR/Instructions.h#L775

1464

Fair enough

mlir/test/Dialect/LLVMIR/roundtrip.mlir

224–225

Good idea

226

This was leftover from an earlier attempt to implement a volatile attribute to match the LLVM IR. However, this seems impossible in the llvm builder's present state. There's no way it seems to avoid C++ keyword issues. If I had access to a special $_op expansion then I could write a builder that calls on the op directly to access the attribute.

For now I'll drop the volatile in this test and we can figure out a plan on how to support C++ keyword conflicts more generally.

[ftynse]

mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td

766

Works for me

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp

1433

This code is older than that function. You are very welcome to modernize the rest in a separate diff :)

1448

parser.parseAttribute(ordering, "ordering", result.attributes)and so on, using result.attributes instead of attrs.

1454

I don't expect much change happening to this part any time soon, but okay to keep as is.

[jyasskin]

mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td

751

Since LLVM needed to support Java as well as C++, I felt that relaxed didn't do a good enough job of expressing the difference between their lowest levels of atomicity. I drew "monotonic" from the 4th portability assumption in the "correctness argument" in http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2007/n2325.html (though that paper doesn't actually suggest using the term as a name for anything). Java non-volatile variables provide defined behavior for racy access, but they don't provide this property, so they got the unordered term.

[flaub]

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp

1448

The issue is that we need to convert from a string representation to an integer one.

However, I think I've cleaned it up by parsing into a StringAttr and then doing the conversion followed by:

result.addAttribute("bin_op", binOpAttr);

[flaub]

• CR changes

[merge_guards_bot]

Unit tests: pass. 61744 tests passed, 0 failed and 780 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[nicolasvasilache]

From a cursory glance this looks good, I'd use the capital name for enums that conflict with C/C++ keywords.

I'm also very +1 on mirroring LLVM and staying in sync with https://github.com/llvm/llvm-project/blob/master/llvm/include/llvm/Support/AtomicOrdering.h#L81.
It makes sense that MLIR would be exactly as "wrong" as LLVM in unsurprising ways.
I feel that developers of frontend X should not have to go read the C++ standard or papers about C++ and Java to understand why there is a surprising mismatch between LLVM and MLIR.

But it seems this would be a good topic to bring to the LLVM community as a whole and then MLIR can just follow?

[ftynse]

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp

1448

result.attributes is literally a vector of named attributes (https://github.com/llvm/llvm-project/blob/e19188af0a2690f222db7d8b866be0afef7b3da0/mlir/include/mlir/IR/OperationSupport.h#L267), fully redundant with the vector attrs you declared locally.

If you were able to do attrs[0].second = <..>, you can also do result.attributes[0].second = <..>

mlir/test/Dialect/LLVMIR/roundtrip.mlir

223

Sorry for not noticing earlier, but please provide the tests for parser error messages in mlir/test/Dialect/LLVMIR/invalid.mlir as well.

mlir/test/Target/llvmir.mlir

1074

I would test each bin_op and each ordering at least once. It's mechanical but guards against subtle syntactic changes.

[mehdi_amini]

mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td

751

Could you call this one relaxed, so it matches C++, instead of the LLVM IR name?

@jfb : could we rename the LLVM IR name to align everyone on the C++ name?

[jfb]

mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td

751

@jfb : could we rename the LLVM IR name to align everyone on the C++ name?

I think that would make sense, as long as @reames concurs. It's always seemed to be a minor mistake to me, it adds confusion when handling atomics inside of LLVM IR, and it would be a shame for a new IR to mimic it for consistency's sake.

That being said, it's by far not the most confusing thing in LLVM :)

[flaub]

• CR changes

[flaub]

More CR changes incoming

[merge_guards_bot]

Unit tests: pass. 61744 tests passed, 0 failed and 780 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[flaub]

@ftynse I believe this is ready to go, anything else you'd like?

Also, I don't yet have committer access, so I'll need some help getting it landed.

[jfb]

In D72741#1826938, @flaub wrote:

@ftynse I believe this is ready to go, anything else you'd like?

Also, I don't yet have committer access, so I'll need some help getting it landed.

Please resolve the question I asked, either here or with a follow-up plan. @mehdi_amini's suggestion was good.

[flaub]

I don't think this commit should be about making changes to LLVM IR. I can do that in a followup, but this commit should be in sync with whatever LLVM IR is today. There seems to be consensus with this approach.

[flaub]

@jfb What process do I need to follow to ensure that any changes I make to LLVM IR doesn't break the world? This would include updates to the documentation. I'm new here and I'm not sure which tests I should focus on and where the docs live.

[jfb]

In D72741#1826996, @flaub wrote:

I don't think this commit should be about making changes to LLVM IR. I can do that in a followup, but this commit should be in sync with whatever LLVM IR is today. There seems to be consensus with this approach.

Agreed this shouldn't make changes to LLVM IR. However, I'd like to understand whether:

1. a change would occur here first (in this patch, to MLIR), and then in LLVM IR (and who would lead this)
2. in MLIR and LLVM IR at the same time, as a follow-up, and who would lead this
3. never

You're making a new IR, and that's exactly the right time to revisit basic things like this instead of copying LLVM IR. This is so minor, I'm baffled by the pushback. You don't have consensus as evidenced by this discussion, and unwillingness to engage in such a basic question is super worrying. Please don't copy LLVM IR and instead understand why it's designed in a certain manner and what mistakes (however minor) it might contain. I'm not blocking this patch indefinitely, I'm asking you to answer these questions before proceeding. That should take a few minutes.

[ftynse]

Changing LLVM IR should _not_ be in this commit, it's semantically different and requires separate discussion with relevant people involved. I also don't see a compelling reason to block this from landing until LLVM IR changes (or decides not to). LLVM dialect models LLVM IR in its current state, whatever the state is.

[flaub]

I'm happy to do whatever the community wants to do. I was under the impression from @ftynse that we shouldn't be changing the MLIR IR, that it should match exactly whatever the LLVM IR is. It's unclear to me what the ramifications of making such a change in LLVM IR are, so I don't feel qualified at this time to make such a change. But, I could certainly take some guidance on this. (this is where my pushback is coming from, sorry if this came off the wrong way).

[ftynse]

To clarify further, LLVM dialect in MLIR is not a new IR, it's essentially a representation of LLVM IR using MLIR. It's supposed to minimize the cost of translation from MLIR to LLVM IR. While it does diverge from LLVM IR proper, it does so for modeling reasons: there's no first-class constants in MLIR, neither are there phi nodes. Changing the name of an enumerand locally to the LLVM dialect will increase the cost of translation, which goes against the goal of this dialect. That being said, I do agree that we should use this opportunity to reflect on LLVM IR design and update it following the proper process.

[mehdi_amini]

In D72741#1826956, @jfb wrote:

In D72741#1826938, @flaub wrote:

@ftynse I believe this is ready to go, anything else you'd like?

Also, I don't yet have committer access, so I'll need some help getting it landed.

Please resolve the question I asked, either here or with a follow-up plan. @mehdi_amini's suggestion was good.

I wasn't intending to block this revision, I am aligned with @ftynse and @nicolasvasilache that the LLVM dialect in MLIR is intended to mirror as close as possible LLVM IR and so aligning the naming makes sense to me.

I am very supportive to take the opportunity of revisions like the current one to surface issues with LLVM and work towards fixing them, I don't think we need to block the progress on the work on completing the MLIR<->LLVM mapping though.
I was following up on your question with this mindset.

[mehdi_amini]

In D72741#1827039, @jfb wrote:

You're making a new IR, and that's exactly the right time to revisit basic things like this instead of copying LLVM IR.

Maybe what is causing confusion is that we also have the MLIR standard dialect, which is more of a new IR itself. We are actually quite careful about things going in the standard dialect and it isn't bound by LLVM IR (other that we try to think about how to map down to LLVM).

Another way to put it is that the LLVM Dialect is only a layer intended to be hiding the LLVM IR Builders: while in MLIR you lower down to the LLVM Dialect instead of emitting directly LLVM IR. The reason is that this composes better (you can lower progressively to LLVM and not as a one shot conversion). So the LLVM dialect "embeds" the LLVM IR inside MLIR.

[jfb]

In D72741#1827085, @mehdi_amini wrote:

In D72741#1827039, @jfb wrote:

You're making a new IR, and that's exactly the right time to revisit basic things like this instead of copying LLVM IR.

Maybe what is causing confusion is that we also have the MLIR standard dialect, which is more of a new IR itself. We are actually quite careful about things going in the standard dialect and it isn't bound by LLVM IR (other that we try to think about how to map down to LLVM).

Another way to put it is that the LLVM Dialect is only a layer intended to be hiding the LLVM IR Builders: while in MLIR you lower down to the LLVM Dialect instead of emitting directly LLVM IR. The reason is that this composes better (you can lower progressively to LLVM and not as a one shot conversion). So the LLVM dialect "embeds" the LLVM IR inside MLIR.

@flaub and I talked over Discord, and I think we're on the same page! Thanks Mehdi as well. Let's move this forward, and have the naming discussion separately.

[tra]

Oops. Herald rule misfired.

[ftynse]

@tra, your rule adds you as blocking on all of MLIR :)

[flaub]

Thanks everyone!

[tra]

In D72741#1827150, @ftynse wrote:

@tra, your rule adds you as blocking on all of MLIR :)

Sorry about the noise. Should be fixed now. I didn't realize that 'blocking reviewer' was the default action on herald rules.

mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td

def LLVM_Prefetch : LLVM_ZeroResultOp<"intr.prefetch">,
   let llvmBuilder = [{
     llvm::Module *module = builder.GetInsertBlock()->getModule();
     llvm::Function *fn = llvm::Intrinsic::getDeclaration(
         module, llvm::Intrinsic::prefetch, $addr->getType());
     builder.CreateCall(fn, {$addr, $rw, $hint, $cache});
   }];
 }
 
+def AtomicBinOpXchg : I64EnumAttrCase<"xchg", 0>;
+def AtomicBinOpAdd  : I64EnumAttrCase<"add", 1>;
+def AtomicBinOpSub  : I64EnumAttrCase<"sub", 2>;
+def AtomicBinOpAnd  : I64EnumAttrCase<"_and", 3>;
+def AtomicBinOpNand : I64EnumAttrCase<"nand", 4>;
+def AtomicBinOpOr   : I64EnumAttrCase<"_or", 5>;
+def AtomicBinOpXor  : I64EnumAttrCase<"_xor", 6>;
+def AtomicBinOpMax  : I64EnumAttrCase<"max", 7>;
+def AtomicBinOpMin  : I64EnumAttrCase<"min", 8>;
+def AtomicBinOpUMax : I64EnumAttrCase<"umax", 9>;
+def AtomicBinOpUMin : I64EnumAttrCase<"umin", 10>;
+def AtomicBinOpFAdd : I64EnumAttrCase<"fadd", 11>;
+def AtomicBinOpFSub : I64EnumAttrCase<"fsub", 12>;
+def AtomicBinOp : I64EnumAttr<
+    "AtomicBinOp",
+    "llvm.atomicrmw binary operations",
+    [AtomicBinOpXchg, AtomicBinOpAdd, AtomicBinOpSub, AtomicBinOpAnd,
+     AtomicBinOpNand, AtomicBinOpOr, AtomicBinOpXor, AtomicBinOpMax,
+     AtomicBinOpMin, AtomicBinOpUMax, AtomicBinOpUMin, AtomicBinOpFAdd,
+     AtomicBinOpFSub]> {
+  let cppNamespace = "::mlir::LLVM";
+}
+
+def AtomicOrderingNotAtomic              : I64EnumAttrCase<"not_atomic", 0>;
+def AtomicOrderingUnordered              : I64EnumAttrCase<"unordered", 1>;
+def AtomicOrderingMonotonic              : I64EnumAttrCase<"monotonic", 2>;
+def AtomicOrderingAcquire                : I64EnumAttrCase<"acquire", 4>;
+def AtomicOrderingRelease                : I64EnumAttrCase<"release", 5>;
+def AtomicOrderingAcquireRelease         : I64EnumAttrCase<"acq_rel", 6>;
+def AtomicOrderingSequentiallyConsistent : I64EnumAttrCase<"seq_cst", 7>;
+def AtomicOrdering : I64EnumAttr<
+    "AtomicOrdering",
+    "Atomic ordering for LLVM's memory model",
+    [AtomicOrderingNotAtomic, AtomicOrderingUnordered, AtomicOrderingMonotonic,
+     AtomicOrderingAcquire, AtomicOrderingRelease, AtomicOrderingAcquireRelease,
+     AtomicOrderingSequentiallyConsistent]> {
+  let cppNamespace = "::mlir::LLVM";
+}
+
+def LLVM_AtomicRMWOp : LLVM_Op<"atomicrmw">,
+    Arguments<(ins AtomicBinOp:$bin_op, LLVM_Type:$ptr, LLVM_Type:$val,
+                   AtomicOrdering:$ordering)>,
+    Results<(outs LLVM_Type:$res)> {
+  let llvmBuilder = [{
+    $res = builder.CreateAtomicRMW(getLLVMAtomicBinOp($bin_op), $ptr, $val,
+                                   getLLVMAtomicOrdering($ordering));
+  }];
+  let parser = [{ return parseAtomicRMWOp(parser, result); }];
+  let printer = [{ printAtomicRMWOp(p, *this); }];
+  let verifier = "return ::verify(*this);";
+}
+
 #endif // LLVMIR_OPS

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp

 static LogicalResult verify(LLVM::NullOp op) {
   auto llvmType = op.getType().dyn_cast<LLVM::LLVMType>();
   if (!llvmType || !llvmType.isPointerTy())
     return op.emitOpError("expected LLVM IR pointer type");
   return success();
 }
 
 //===----------------------------------------------------------------------===//
+// Printer, parser and verifier for LLVM::AtomicRMWOp.
+//===----------------------------------------------------------------------===//
+
+static void printAtomicRMWOp(OpAsmPrinter &p, AtomicRMWOp &op) {
+  p << op.getOperationName() << " ";
+  p << '"' << stringifyAtomicBinOp(op.bin_op()) << "\" ";
+  p << '"' << stringifyAtomicOrdering(op.ordering()) << "\" ";
+  p << op.ptr() << ", " << op.val();
+  p.printOptionalAttrDict(op.getAttrs(), {"bin_op", "ordering"});
+  p << " : (" << op.ptr().getType() << ", " << op.val().getType() << ") -> "
+    << op.res().getType();
+}
+
+// <operation> ::= `llvm.atomicrmw` string-literal string-literal
+//                 ssa-use `,` ssa-use attribute-dict? `:` type
+static ParseResult parseAtomicRMWOp(OpAsmParser &parser,
+                                    OperationState &result) {
+  Type type;
+  StringAttr binOp, ordering;
+  llvm::SMLoc binOpLoc, orderingLoc, trailingTypeLoc;
+  OpAsmParser::OperandType ptr, val;
+  if (parser.getCurrentLocation(&binOpLoc) ||
+      parser.parseAttribute(binOp, "bin_op", result.attributes) ||
+      parser.getCurrentLocation(&orderingLoc) ||
+      parser.parseAttribute(ordering, "ordering", result.attributes) ||
+      parser.parseOperand(ptr) || parser.parseComma() ||
+      parser.parseOperand(val) ||
+      parser.parseOptionalAttrDict(result.attributes) || parser.parseColon() ||
+      parser.getCurrentLocation(&trailingTypeLoc) || parser.parseType(type))
+    return failure();
+
+  // Extract the result type from the trailing function type.
+  auto funcType = type.dyn_cast<FunctionType>();
+  if (!funcType || funcType.getNumInputs() != 2 ||
+      funcType.getNumResults() != 1)
+    return parser.emitError(
+        trailingTypeLoc,
+        "expected trailing function type with two arguments and one result");
+
+  if (parser.resolveOperand(ptr, funcType.getInput(0), result.operands) ||
+      parser.resolveOperand(val, funcType.getInput(1), result.operands))
+    return failure();
+
+  // Replace the string attribute `bin_op` with an integer attribute.
+  auto binOpKind = symbolizeAtomicBinOp(binOp.getValue());
+  if (!binOpKind) {
+    return parser.emitError(binOpLoc)
+           << "'" << binOp.getValue()
+           << "' is an incorrect value of the 'bin_op' attribute";
+  }
+
+  auto binOpValue = static_cast<int64_t>(binOpKind.getValue());
+  auto binOpAttr = parser.getBuilder().getI64IntegerAttr(binOpValue);
+  result.attributes[0].second = binOpAttr;
+
+  // Replace the string attribute `ordering` with an integer attribute.
+  auto orderingKind = symbolizeAtomicOrdering(ordering.getValue());
+  if (!orderingKind) {
+    return parser.emitError(orderingLoc)
+           << "'" << ordering.getValue()
+           << "' is an incorrect value of the 'ordering' attribute";
+  }
+
+  auto orderingValue = static_cast<int64_t>(orderingKind.getValue());
+  auto orderingAttr = parser.getBuilder().getI64IntegerAttr(orderingValue);
+  result.attributes[1].second = orderingAttr;
+
+  result.addTypes(funcType.getResults());
+  return success();
+}
+
+static LogicalResult verify(AtomicRMWOp op) {
+  auto ptrType = op.ptr().getType().cast<LLVM::LLVMType>();
+  if (!ptrType.isPointerTy())
+    return op.emitOpError("expected LLVM IR pointer type for operand #0");
+  auto valType = op.val().getType().cast<LLVM::LLVMType>();
+  if (valType != ptrType.getPointerElementTy())
+    return op.emitOpError("expected LLVM IR element type for operand #0 to "
+                          "match type for operand #1");
+  auto resType = op.res().getType().cast<LLVM::LLVMType>();
+  if (resType != valType)
+    return op.emitOpError(
+        "expected LLVM IR result type to match type for operand #1");
+  if (op.bin_op() == AtomicBinOp::fadd || op.bin_op() == AtomicBinOp::fsub) {
+    if (!valType.getUnderlyingType()->isFloatingPointTy())
+      return op.emitOpError("expected LLVM IR floating point type");
+  } else if (op.bin_op() == AtomicBinOp::xchg) {
+    if (!valType.isIntegerTy(8) && !valType.isIntegerTy(16) &&
+        !valType.isIntegerTy(32) && !valType.isIntegerTy(64) &&
+        !valType.getUnderlyingType()->isHalfTy() && !valType.isFloatTy() &&
+        !valType.isDoubleTy())
+      return op.emitOpError("unexpected LLVM IR type for 'xchg' bin_op");
+  } else {
+    if (!valType.isIntegerTy(8) && !valType.isIntegerTy(16) &&
+        !valType.isIntegerTy(32) && !valType.isIntegerTy(64))
+      return op.emitOpError("expected LLVM IR integer type");
+  }
+  return success();
+}
+
+//===----------------------------------------------------------------------===//
 // LLVMDialect initialization, type parsing, and registration.
 //===----------------------------------------------------------------------===//
 
 namespace mlir {
 namespace LLVM {
 namespace detail {
 struct LLVMDialectImpl {
   LLVMDialectImpl() : module("LLVMDialectModule", llvmContext) {}
 
   llvm::LLVMContext llvmContext;
   llvm::Module module;
 
   /// A set of LLVMTypes that are cached on construction to avoid any lookups or
   /// locking.
   LLVMType int1Ty, int8Ty, int16Ty, int32Ty, int64Ty, int128Ty;

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp

static llvm::CmpInst::Predicate getLLVMCmpPredicate(FCmpPredicate p) {
   case LLVM::FCmpPredicate::uno:
     return llvm::CmpInst::Predicate::FCMP_UNO;
   case LLVM::FCmpPredicate::_true:
     return llvm::CmpInst::Predicate::FCMP_TRUE;
   }
   llvm_unreachable("incorrect comparison predicate");
 }
 
+static llvm::AtomicRMWInst::BinOp getLLVMAtomicBinOp(AtomicBinOp op) {
+  switch (op) {
+  case LLVM::AtomicBinOp::xchg:
+    return llvm::AtomicRMWInst::BinOp::Xchg;
+  case LLVM::AtomicBinOp::add:
+    return llvm::AtomicRMWInst::BinOp::Add;
+  case LLVM::AtomicBinOp::sub:
+    return llvm::AtomicRMWInst::BinOp::Sub;
+  case LLVM::AtomicBinOp::_and:
+    return llvm::AtomicRMWInst::BinOp::And;
+  case LLVM::AtomicBinOp::nand:
+    return llvm::AtomicRMWInst::BinOp::Nand;
+  case LLVM::AtomicBinOp::_or:
+    return llvm::AtomicRMWInst::BinOp::Or;
+  case LLVM::AtomicBinOp::_xor:
+    return llvm::AtomicRMWInst::BinOp::Xor;
+  case LLVM::AtomicBinOp::max:
+    return llvm::AtomicRMWInst::BinOp::Max;
+  case LLVM::AtomicBinOp::min:
+    return llvm::AtomicRMWInst::BinOp::Min;
+  case LLVM::AtomicBinOp::umax:
+    return llvm::AtomicRMWInst::BinOp::UMax;
+  case LLVM::AtomicBinOp::umin:
+    return llvm::AtomicRMWInst::BinOp::UMin;
+  case LLVM::AtomicBinOp::fadd:
+    return llvm::AtomicRMWInst::BinOp::FAdd;
+  case LLVM::AtomicBinOp::fsub:
+    return llvm::AtomicRMWInst::BinOp::FSub;
+  }
+  llvm_unreachable("incorrect atomic binary operator");
+}
+
+static llvm::AtomicOrdering getLLVMAtomicOrdering(AtomicOrdering ordering) {
+  switch (ordering) {
+  case LLVM::AtomicOrdering::not_atomic:
+    return llvm::AtomicOrdering::NotAtomic;
+  case LLVM::AtomicOrdering::unordered:
+    return llvm::AtomicOrdering::Unordered;
+  case LLVM::AtomicOrdering::monotonic:
+    return llvm::AtomicOrdering::Monotonic;
+  case LLVM::AtomicOrdering::acquire:
+    return llvm::AtomicOrdering::Acquire;
+  case LLVM::AtomicOrdering::release:
+    return llvm::AtomicOrdering::Release;
+  case LLVM::AtomicOrdering::acq_rel:
+    return llvm::AtomicOrdering::AcquireRelease;
+  case LLVM::AtomicOrdering::seq_cst:
+    return llvm::AtomicOrdering::SequentiallyConsistent;
+  }
+  llvm_unreachable("incorrect atomic ordering");
+}
+
 /// Given a single MLIR operation, create the corresponding LLVM IR operation
 /// using the `builder`.  LLVM IR Builder does not have a generic interface so
 /// this has to be a long chain of `if`s calling different functions with a
 /// different number of arguments.
 LogicalResult ModuleTranslation::convertOperation(Operation &opInst,
                                                   llvm::IRBuilder<> &builder) {
   auto extractPosition = [](ArrayAttr attr) {
     SmallVector<unsigned, 4> position;

mlir/test/Dialect/LLVMIR/invalid.mlir

 // FIXME: the LLVM-IR dialect should parse mutually recursive types
 // CHECK-LABEL: @recursive_type
 // expected-error@+1 {{expected end of string}}
 llvm.func @recursive_type(%a : !llvm<"%a = type { %a* }">) ->
                           !llvm<"%a = type { %a* }"> {
   llvm.return %a : !llvm<"%a = type { %a* }">
 }
 
+// -----
+
+// CHECK-LABEL: @atomicrmw_expected_ptr
+func @atomicrmw_expected_ptr(%f32 : !llvm.float) {
+  // expected-error@+1 {{expected LLVM IR pointer type for operand #0}}
+  %0 = llvm.atomicrmw "fadd" "unordered" %f32, %f32 : (!llvm.float, !llvm.float) -> !llvm.float
+  llvm.return
+}
+
+// -----
+// CHECK-LABEL: @atomicrmw_mismatched_operands
+func @atomicrmw_mismatched_operands(%f32_ptr : !llvm<"float*">, %i32 : !llvm.i32) {
+  // expected-error@+1 {{expected LLVM IR element type for operand #0 to match type for operand #1}}
+  %0 = llvm.atomicrmw "fadd" "unordered" %f32_ptr, %i32 : (!llvm<"float*">, !llvm.i32) -> !llvm.float
+  llvm.return
+}
+
+// -----
+// CHECK-LABEL: @atomicrmw_mismatched_result
+func @atomicrmw_mismatched_operands(%f32_ptr : !llvm<"float*">, %f32 : !llvm.float) {
+  // expected-error@+1 {{expected LLVM IR result type to match type for operand #1}}
+  %0 = llvm.atomicrmw "fadd" "unordered" %f32_ptr, %f32 : (!llvm<"float*">, !llvm.float) -> !llvm.i32
+  llvm.return
+}
 
+// -----
+// CHECK-LABEL: @atomicrmw_expected_float
+func @atomicrmw_expected_float(%i32_ptr : !llvm<"i32*">, %i32 : !llvm.i32) {
+  // expected-error@+1 {{expected LLVM IR floating point type}}
+  %0 = llvm.atomicrmw "fadd" "unordered" %i32_ptr, %i32 : (!llvm<"i32*">, !llvm.i32) -> !llvm.i32
+  llvm.return
+}
+
+// -----
+// CHECK-LABEL: @atomicrmw_unexpected_xchg_type
+func @atomicrmw_xchg_type(%i1_ptr : !llvm<"i1*">, %i1 : !llvm.i1) {
+  // expected-error@+1 {{unexpected LLVM IR type for 'xchg' bin_op}}
+  %0 = llvm.atomicrmw "xchg" "unordered" %i1_ptr, %i1 : (!llvm<"i1*">, !llvm.i1) -> !llvm.i1
+  llvm.return
+}
+
+// -----
+// CHECK-LABEL: @atomicrmw_expected_int
+func @atomicrmw_expected_int(%f32_ptr : !llvm<"float*">, %f32 : !llvm.float) {
+  // expected-error@+1 {{expected LLVM IR integer type}}
+  %0 = llvm.atomicrmw "max" "unordered" %f32_ptr, %f32 : (!llvm<"float*">, !llvm.float) -> !llvm.float
+  llvm.return
+}

mlir/test/Dialect/LLVMIR/roundtrip.mlir

 // CHECK-LABEL: @null
 func @null() {
   // CHECK: llvm.mlir.null : !llvm<"i8*">
   %0 = llvm.mlir.null : !llvm<"i8*">
   // CHECK: llvm.mlir.null : !llvm<"{ void (i32, void ()*)*, i64 }*">
   %1 = llvm.mlir.null : !llvm<"{void(i32, void()*)*, i64}*">
   llvm.return
 }
+
+// CHECK-LABEL: @atomics
+func @atomics(%arg0 : !llvm<"float*">, %arg1 : !llvm.float) {
+  // CHECK: llvm.atomicrmw "fadd" "unordered" %{{.*}}, %{{.*}} : (!llvm<"float*">, !llvm.float) -> !llvm.float
+  %0 = llvm.atomicrmw "fadd" "unordered" %arg0, %arg1 : (!llvm<"float*">, !llvm.float) -> !llvm.float
+  llvm.return
+}

mlir/test/Target/llvmir.mlir

llvm.func @null() -> !llvm<"i32*"> {
   %0 = llvm.mlir.null : !llvm<"i32*">
   // CHECK: ret i32* null
   llvm.return %0 : !llvm<"i32*">
 }
 
 // Check that dense elements attributes are exported properly in constants.
 // CHECK-LABEL: @elements_constant_3d_vector
 llvm.func @elements_constant_3d_vector() -> !llvm<"[2 x [2 x <2 x i32>]]"> {
   // CHECK: ret [2 x [2 x <2 x i32>]]
   // CHECK-SAME: {{\[}}[2 x <2 x i32>] [<2 x i32> <i32 1, i32 2>, <2 x i32> <i32 3, i32 4>],
   // CHECK-SAME:       [2 x <2 x i32>] [<2 x i32> <i32 42, i32 43>, <2 x i32> <i32 44, i32 45>]]
   %0 = llvm.mlir.constant(dense<[[[1, 2], [3, 4]], [[42, 43], [44, 45]]]> : vector<2x2x2xi32>) : !llvm<"[2 x [2 x <2 x i32>]]">
   llvm.return %0 : !llvm<"[2 x [2 x <2 x i32>]]">
 }
 
 // CHECK-LABEL: @elements_constant_3d_array
 llvm.func @elements_constant_3d_array() -> !llvm<"[2 x [2 x [2 x i32]]]"> {
   // CHECK: ret [2 x [2 x [2 x i32]]]
   // CHECK-SAME: {{\[}}[2 x [2 x i32]] {{\[}}[2 x i32] [i32 1, i32 2], [2 x i32] [i32 3, i32 4]],
   // CHECK-SAME:       [2 x [2 x i32]] {{\[}}[2 x i32] [i32 42, i32 43], [2 x i32] [i32 44, i32 45]]]
   %0 = llvm.mlir.constant(dense<[[[1, 2], [3, 4]], [[42, 43], [44, 45]]]> : tensor<2x2x2xi32>) : !llvm<"[2 x [2 x [2 x i32]]]">
   llvm.return %0 : !llvm<"[2 x [2 x [2 x i32]]]">
 }
+
+// CHECK-LABEL: @atomics
+llvm.func @atomics(
+    %f32_ptr : !llvm<"float*">, %f32 : !llvm.float,
+    %i32_ptr : !llvm<"i32*">, %i32 : !llvm.i32) -> !llvm.float {
+  // CHECK: atomicrmw fadd float* %{{.*}}, float %{{.*}} unordered
+  %0 = llvm.atomicrmw "fadd" "unordered" %f32_ptr, %f32 : (!llvm<"float*">, !llvm.float) -> !llvm.float
+  // CHECK: atomicrmw fsub float* %{{.*}}, float %{{.*}} unordered
+  %1 = llvm.atomicrmw "fsub" "unordered" %f32_ptr, %f32 : (!llvm<"float*">, !llvm.float) -> !llvm.float
+  // CHECK: atomicrmw xchg float* %{{.*}}, float %{{.*}} monotonic
+  %2 = llvm.atomicrmw "xchg" "monotonic" %f32_ptr, %f32 : (!llvm<"float*">, !llvm.float) -> !llvm.float
+  // CHECK: atomicrmw add i32* %{{.*}}, i32 %{{.*}} acquire
+  %3 = llvm.atomicrmw "add" "acquire" %i32_ptr, %i32 : (!llvm<"i32*">, !llvm.i32) -> !llvm.i32
+  // CHECK: atomicrmw sub i32* %{{.*}}, i32 %{{.*}} release
+  %4 = llvm.atomicrmw "sub" "release" %i32_ptr, %i32 : (!llvm<"i32*">, !llvm.i32) -> !llvm.i32
+  // CHECK: atomicrmw and i32* %{{.*}}, i32 %{{.*}} acq_rel
+  %5 = llvm.atomicrmw "_and" "acq_rel" %i32_ptr, %i32 : (!llvm<"i32*">, !llvm.i32) -> !llvm.i32
+  // CHECK: atomicrmw nand i32* %{{.*}}, i32 %{{.*}} seq_cst
+  %6 = llvm.atomicrmw "nand" "seq_cst" %i32_ptr, %i32 : (!llvm<"i32*">, !llvm.i32) -> !llvm.i32
+  // CHECK: atomicrmw or i32* %{{.*}}, i32 %{{.*}} unordered
+  %7 = llvm.atomicrmw "_or" "unordered" %i32_ptr, %i32 : (!llvm<"i32*">, !llvm.i32) -> !llvm.i32
+  // CHECK: atomicrmw xor i32* %{{.*}}, i32 %{{.*}} unordered
+  %8 = llvm.atomicrmw "_xor" "unordered" %i32_ptr, %i32 : (!llvm<"i32*">, !llvm.i32) -> !llvm.i32
+  // CHECK: atomicrmw max i32* %{{.*}}, i32 %{{.*}} unordered
+  %9 = llvm.atomicrmw "max" "unordered" %i32_ptr, %i32 : (!llvm<"i32*">, !llvm.i32) -> !llvm.i32
+  // CHECK: atomicrmw min i32* %{{.*}}, i32 %{{.*}} unordered
+  %10 = llvm.atomicrmw "min" "unordered" %i32_ptr, %i32 : (!llvm<"i32*">, !llvm.i32) -> !llvm.i32
+  // CHECK: atomicrmw umax i32* %{{.*}}, i32 %{{.*}} unordered
+  %11 = llvm.atomicrmw "umax" "unordered" %i32_ptr, %i32 : (!llvm<"i32*">, !llvm.i32) -> !llvm.i32
+  // CHECK: atomicrmw umin i32* %{{.*}}, i32 %{{.*}} unordered
+  %12 = llvm.atomicrmw "umin" "unordered" %i32_ptr, %i32 : (!llvm<"i32*">, !llvm.i32) -> !llvm.i32
+  llvm.return %0 : !llvm.float
+}