rnk added inline comments.
================ Comment at: include/clang/Basic/BuiltinsARM.def:270 +TARGET_HEADER_BUILTIN(_InterlockedCompareExchange64_nf, "LLiLLiD*LLiLLi", "nh", "intrin.h", ALL_MS_LANGUAGES, "") +TARGET_HEADER_BUILTIN(_InterlockedCompareExchange64_rel, "LLiLLiD*LLiLLi", "nh", "intrin.h", ALL_MS_LANGUAGES, "") + ---------------- mgrang wrote: > rnk wrote: > > Given how much duplication there is, I think we need to have some kind of > > BuiltinsMSVC.def that contains a list of all the interlocked builitin > > families, like this: > > ``` > > INTERLOCKED_BUILTIN(_InterlockedCompareExchange) > > INTERLOCKED_BUILTIN(_InterlockedOr) > > INTERLOCKED_BUILTIN(_InterlockedAnd) > > ``` > > We'd include this file here, with INTERLOCKED_BUILTIN defined as: > > ``` > > #define INTERLOCKED_BUILTIN(Op) \ > > TARGET_HEADER_BUILTIN(Op##8_acq, "ccD*cc", "nh", "intrin.h", > > ALL_MS_LANGUAGES, "") \ > > TARGET_HEADER_BUILTIN(Op##8_nf, "ccD*cc", "nh", "intrin.h", > > ALL_MS_LANGUAGES, "") \ > > ... > > ``` > > That'll stamp out the enums that we need, and then we can reuse the .def > > file to reduce duplication in the switch/case below. > > > > Every op is basically 16 operations: {8, 16, 32, 64} X {seq_cst, rel, acq, > > nf} > > > > I'm not sure what to do about the ones that overlap with x86. > Thanks Reid. Yes, I think we can certainly cleanup further. That being said, > can we get these patches committed first and then cleanup in a follow-up > patch? Fair enough. I think the approach you have in all of these patches is reasonable and not hard to clean up later. I don't have time to review each IR implementation right now, but it is important to get a second opinion, because as you've seen they have had bugs (missing fences) in the past. ================ Comment at: lib/CodeGen/CGBuiltin.cpp:259-262 + // For Release ordering, the failure ordering should be Monotonic. + auto FailureOrdering = SuccessOrdering == AtomicOrdering::Release ? + AtomicOrdering::Monotonic : + SuccessOrdering; ---------------- I don't know enough about the memory model to really say if this is right, so I'll pass the buck to @efriedma. Repository: rC Clang https://reviews.llvm.org/D54062 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits