[PATCH] D54062: [COFF, ARM64] Implement InterlockedCompareExchange*_* builtins

[Reid Kleckner via Phabricator via cfe-commits]

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