This is an archive of the discontinued LLVM Phabricator instance.

Bugfix for Codegen of atomic load/store ops.
ClosedPublic

Authored by ABataev on Dec 3 2014, 3:01 AM.

Details

Summary

Currently clang fires assertions on x86-64 on atomic load/store operations for long double operands. Patch fixes codegen for such operations.

Diff Detail

Event Timeline

ABataev updated this revision to Diff 16857.Dec 3 2014, 3:01 AM
ABataev retitled this revision from to Bugfix for Codegen of atomic load/store ops..
ABataev updated this object.
ABataev edited the test plan for this revision. (Show Details)
ABataev added reviewers: rjmccall, hfinkel.
ABataev added a subscriber: Unknown Object (MLST).
rjmccall edited edge metadata.Dec 3 2014, 12:35 PM

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

You need to be more specific in this test. You should be able to match the alloca.

14

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.

John, thanks for the review! Also I need to add a test for 32 bit mode (long double has size 96 bits there) and check that in this mode we create a libcall.

test/CodeGen/x86_64-atomic-long_double.c
7

Ok, I'll improve test

14

Ok, I'll add additional checks

ABataev updated this revision to Diff 17037.Dec 8 2014, 4:24 AM
ABataev edited edge metadata.

Rework after review.
Improved test and fixed bugs in codegen.

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
1047

Consider making this a method on the AtomicInfo.

1230

Inst->setVolatile(Obj.isVolatileQualified());
Inst->setWeak(IsWeak);

1238

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.

John, Ok, I'll split this patch in 2 parts.

lib/CodeGen/CGAtomic.cpp
1047

Ok, done.

1230

Done

1238

I reworked it.

ABataev updated this revision to Diff 17072.Dec 8 2014, 11:39 PM

Update after review.

Thanks for splitting the patch. Generally looks good. One comment:

lib/CodeGen/CGAtomic.cpp
973

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
973

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.

rjmccall added inline comments.Dec 11 2014, 1:48 PM
lib/CodeGen/CGAtomic.cpp
973

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.

ABataev added inline comments.Dec 12 2014, 4:04 AM
lib/CodeGen/CGAtomic.cpp
973

Ok, will do

ABataev updated this revision to Diff 17226.Dec 12 2014, 5:03 AM

Update after review

Looks great, thanks.

This revision was automatically updated to reflect the committed changes.