Currently clang fires assertions on x86-64 on atomic load/store operations for long double operands. Patch fixes codegen for such operations.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
Thanks for taking this on. Please test all the basic atomic operations, not just loads and stores, and ensure that the extra bits take on some sensible behavior.
test/CodeGen/x86_64-atomic-long_double.c | ||
---|---|---|
7 ↗ | (On Diff #16857) | You need to be more specific in this test. You should be able to match the alloca. |
14 ↗ | (On Diff #16857) | This isn't good enough; x86_fp80's store size doesn't fill its rounded-up size. So you also need to ensure that the extra bits have some consistent value — i.e. zero. In other words, there needs to be a memset of the alloca to zero before you can store the long double into it. |
Hmm. Using the optimized libcalls is a great idea, but it should be a separate patch. You also need to mention that you'd like to do this on cfe-dev first, since it does introduce new runtime dependencies, even if they're a documented part of the existing atomics runtime.
lib/CodeGen/CGAtomic.cpp | ||
---|---|---|
1073 ↗ | (On Diff #17037) | Consider making this a method on the AtomicInfo. |
1274 ↗ | (On Diff #17037) | Inst->setVolatile(Obj.isVolatileQualified()); |
1282 ↗ | (On Diff #17037) | This part should also be a method on the AtomicInfo, or at least its own helper function. convertIntToRValue? We probably have the same code in the atomic-load path. |
Thanks for splitting the patch. Generally looks good. One comment:
lib/CodeGen/CGAtomic.cpp | ||
---|---|---|
985 ↗ | (On Diff #17072) | Is there a good reason that this has to be conditional rather than just being a single convertIntToRValue method? It's okay to pass a ReturnValueSlot in. |
John, here is my answer
lib/CodeGen/CGAtomic.cpp | ||
---|---|---|
985 ↗ | (On Diff #17072) | If some value cannot be bitcasted from Int to ValueType (from Int128 to fp80, for example), we have to create a temp. But we have to create different temps for AtomicLoad() and for AtomicCompareExchange() (here we must create a structure of 2 elements). And these temps must be created only if we were unable to directly cast from int to value type. |
lib/CodeGen/CGAtomic.cpp | ||
---|---|---|
985 ↗ | (On Diff #17072) | Wait, what? Why is EmitAtomicCompareExchange returning a first-class aggregate? Just have it return a std::pair of values. That will also let you return an RValue for the current value, which is probably more generally useful to its callers. |
lib/CodeGen/CGAtomic.cpp | ||
---|---|---|
985 ↗ | (On Diff #17072) | Ok, will do |