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

Repository
rL LLVM

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 ↗(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.

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 ↗(On Diff #16857)

Ok, I'll improve test

14 ↗(On Diff #16857)

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
1073 ↗(On Diff #17037)

Consider making this a method on the AtomicInfo.

1274 ↗(On Diff #17037)

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

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.

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

lib/CodeGen/CGAtomic.cpp
1073 ↗(On Diff #17037)

Ok, done.

1274 ↗(On Diff #17037)

Done

1282 ↗(On Diff #17037)

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

rjmccall added inline comments.Dec 11 2014, 1:48 PM
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.

ABataev added inline comments.Dec 12 2014, 4:04 AM
lib/CodeGen/CGAtomic.cpp
985 ↗(On Diff #17072)

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.