This is an archive of the discontinued LLVM Phabricator instance.

[MLIR] LLVM Dialect: add llvm.cmpxchg and improve llvm.atomicrmw custom parser
ClosedPublic

Authored by flaub on Jan 19 2020, 2:53 AM.

Details

Summary

Add a llvm.cmpxchg op as a counterpart to LLVM IR's cmpxchg instruction.
Note that the weak, 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 (especially for reductions that can't make use
of atomicrmw, e.g. fmax).

Diff Detail

Event Timeline

flaub created this revision.Jan 19 2020, 2:53 AM

Unit tests: pass. 62000 tests passed, 0 failed and 783 were skipped.

clang-tidy: unknown.

clang-format: pass.

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

ftynse accepted this revision.Jan 20 2020, 1:30 AM

Minor comments only.

Thanks!

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
1351–1359

Please add some documentation to this function.

1514

Consider exposing isHalfTy on LLVMType

This revision is now accepted and ready to land.Jan 20 2020, 1:30 AM
jfb added inline comments.Jan 20 2020, 3:40 PM
mlir/test/Dialect/LLVMIR/invalid.mlir
499

That restriction came from C++11 and is no longer in place in C++17:
http://wg21.link/P0418
See note on failure on https://en.cppreference.com/w/cpp/atomic/atomic/compare_exchange

Seems like a bug in LLVM IR to enforce this, would be good if you could fix LLVM IR and not copy that specific mistake :)

flaub marked an inline comment as done.Jan 20 2020, 3:53 PM
flaub added inline comments.
mlir/test/Dialect/LLVMIR/invalid.mlir
499

Thanks for the heads up and references. I can attempt another patch later to bring us up to the C++17 standard. It looks like we'll need to :

  • Update any LLVM IR validation (I haven't looked yet how LLVM IR gets validated, I'm only familiar with MLIR at the moment).
  • Update any documentation (I got the current set of restrictions from https://llvm.org/docs/LangRef.html#cmpxchg-instruction.
  • Remove this restriction from the MLIR LLVM dialect
jfb added inline comments.Jan 20 2020, 4:08 PM
mlir/test/Dialect/LLVMIR/invalid.mlir
499

I think you can move this patch forward without the restriction, and do any LLVM IR changes in parallel? I'm fine with it anyways :)

FWIW even older clang generates code for this in C++11 mode: https://godbolt.org/z/GyiC6Y

flaub updated this revision to Diff 239244.Jan 21 2020, 12:48 AM
  • CR changes
  • Merge remote-tracking branch 'origin/master' into flaub-cmpxchg
flaub marked 4 inline comments as done.Jan 21 2020, 12:50 AM

Unit tests: pass. 62043 tests passed, 0 failed and 783 were skipped.

clang-tidy: unknown.

clang-format: pass.

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

This revision was automatically updated to reflect the committed changes.