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

[David Li via llvm-commits]

davidxl added inline comments.

================
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
----------------
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.



================
Comment at: test/Transforms/AtomicExpand/SPARC/libcalls.ll:77
@@ +76,3 @@
+; CHECK:  %1 = bitcast i128* %arg to i8*
+; CHECK:  %2 = alloca i128, align 16
+; CHECK:  %3 = bitcast i128* %2 to i8*
----------------
jyknight wrote:
> davidxl wrote:
> > are the checks for alloca, bitcast necessary for the test?
> You mean: don't check the whole function output, just some particularly representative lines? I think it's pretty important to check all of the instructions, since each one has some distinct code to emit it.
The problem is that it exposes implementation details which may change in the future. For instance
1) the bitcast may go away
2) even though I understand the need for insertvalue/extractvalue (see the fetchadd impl test below using a loop) in current implementation, strictly speaking, they can in theory be collapsed if the lowering sees a slightly larger scope

These are minor issues that do not need to be fixed IMO -- just a comment.


http://reviews.llvm.org/D18200