[PATCH] D67132: [PATCH] Improve support for atomicrmw and cmpxchg in C API.

Tue Sep 10 11:47:01 PDT 2019

nickwasmer added a comment.

Ping! It's been 1 week since I sent out this patch.

Here's my suggestion for how to review the patch which you may want to follow:
a) check the change to AtomicRMWBinOp. The list should match AtomicRMWInst::BinOp link <https://llvm.org/doxygen/classllvm_1_1AtomicRMWInst.html#adbf3dd67a56ac91e0d98970d31e053c0>
b) check `mapFromLLVMRMWBinOp` and `mapToLLVMRMWBinOp`, ensure they cover all cases and are each other's inverses.
c) the inclusion of `CatchSwitchInst`, `CallBrInst`, `AtomicCmpXchgInst`, `AtomicRMWInst` and `FenceInst` in `LLVM_FOR_EACH_VALUE_SUBCLASS`. This has the effect is creating `LLVMIsACatchSwitchInst(LLVMValueRef)` methods, but little else (nothing else?).
d) `LLVMGetVolatile`, `LLVMSetVolatile`, `LLVMGetOrdering` and `LLVMSetOrdering`. Compare with the LangRef <https://llvm.org/docs/LangRef.html> to make sure those are the only values which have volatile, and the only values which have ordering (note: not including `cmpxchg` which has two orderings and already has its own C API methods).
e) the last pieces which are mechanical: `LLVMGetAtomicRMWBinOp` and `LLVMSetAtomicRMWBinOp` once you know the mappings are correct, `LLVMGetWeak` and `LLVMSetWeak` not to be confused with linkage this sets or clears the weak bit on the `cmpxchg` instruction, ensure no other instruction has one.
f) the test is `llvm-c-test/echo.cpp` where we simply add the `fence` instruction and more setters and getters so that it clones volatile, weak, atomic order, etc.

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

https://reviews.llvm.org/D67132