[PATCH] D18200: Add __atomic_* lowering to AtomicExpandPass.

[James Y Knight via llvm-commits]

jyknight added a comment.

I believe I've addressed as many of the comments as I can. The ones I replied to saying I don't understand remain unresolved.


================
Comment at: lib/CodeGen/AtomicExpandPass.cpp:1110
@@ +1109,3 @@
+  // CAS libcall, via a CAS loop, instead.
+  if (!Success) {
+    expandAtomicRMWToCmpXchg(I, [this](IRBuilder<> &Builder, Value *Addr,
----------------
jyknight wrote:
> reames wrote:
> > This would be much more naturally expressed at the callsite by having this function simply return false when the expansion doesn't work, and then applying the fallback along with the second call.  
> I don't know what you mean by that.
> 
> However, I do see that I can remove the 3 Builder.* calls below by simply calling createCmpXchgInstFun, which is the same thing.
When attempting that change, I remembered why I didn't in the first place: I need the "AtomicCmpXchgInst *Pair" to pass to expandAtomicCASToLibcall, and it's not readily accessible if I just call that other fn. So I've not made any change here.

================
Comment at: lib/CodeGen/AtomicExpandPass.cpp:1144
@@ +1143,3 @@
+
+  unsigned AllocaAlignment = std::min(Size, 16u);
+  bool UseSizedLibcall = canUseSizedAtomicCall(Size, Align, DL);
----------------
jfb wrote:
> Why 16? Could use DL->StackNaturalAlign?
Yeah that seems wrong actually.

The point of setting the alignment is to ensure that the value is aligned sufficiently to be able to cast to an integer of the proper width and load that, as the libcall function may do that. "16" was because that's the maximum specialized size...

But actually, I think it should be using DL.getPrefTypeAlignment(SizedIntTy), so switched to that.

================
Comment at: test/Transforms/AtomicExpand/SPARC/libcalls.ll:10
@@ +9,3 @@
+target datalayout = "e-m:e-p:32:32-i64:64-f128:64-n32-S64"
+target triple = "sparc-unknown-unknown"
+
----------------
davidxl wrote:
> Perhaps move the option to the command line?
I'm not sure that's very useful. It can't really do multiple architectures in one test anyhow, since there's no way to say "REQUIRES:" separately for different run lines, is there?

================
Comment at: test/Transforms/AtomicExpand/SPARC/libcalls.ll:48
@@ +47,3 @@
+; CHECK:  store i16 %old, i16* %2, align 2
+; CHECK:  %4 = call zeroext i1 @__atomic_compare_exchange_2(i8* %1, i8* %3, i16 %new, i32 5, i32 0)
+; CHECK:  %5 = load i16, i16* %2, align 2
----------------
davidxl wrote:
> jyknight wrote:
> > davidxl wrote:
> > > Are these casting from i16 * -> i8 * necessary?
> > > 
> > > Also should __atomoic_compare_exchange_2's  4th argument  be a bool value selecting weak/strong form of compare and exchange ?
> > The casts aren't strictly necessary, in that all pointers are equivalent. But, the code needed to choose something to be the signature of the libcall, and it was easier to just use i8* everywhere (which, btw, is the same thing clang is doing today). When the typeless pointers work is done, that'll go away of course.
> > 
> > No, __atomic_compare_exchange_* don't have a weak/strong argument in the libcall. Only in the frontend builtin.
> From the documentation, the signature of __atomic_compare_exchange_N  takes  iN* as argument type  and it is also how the lib function is implemented.
> 
> 
Whether the pointee types match doesn't really matter, though. Especially as the plan is to remove typed pointers entirely, at which point it'll just be "ptr" or something, it seemed best to just use the same kind always.


http://reviews.llvm.org/D18200