[PATCH] D18200: Add __atomic_* lowering to AtomicExpandPass.
David Li via llvm-commits
llvm-commits at lists.llvm.org
Fri Mar 18 12:53:19 PDT 2016
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
> 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*
> 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.
More information about the llvm-commits