This is an archive of the discontinued LLVM Phabricator instance.

_Atomic of empty struct shouldn't assert
ClosedPublic

Authored by jfb on May 8 2018, 5:18 PM.

Details

Summary

An _Atomic of an empty struct is pretty silly. In general we just widen empty structs to hold a byte's worth of storage, and we represent size and alignment as 0 internally and let LLVM figure out what to do. For _Atomic it's a bit different: the memory model mandates concrete effects occur when atomic operations occur, so in most cases actual instructions need to get emitted. It's really not worth trying to optimize empty struct atomics by figuring out e.g. that a fence would do, even though sane compilers should do optimize atomics. Further, wg21.link/p0528 will fix C++20 atomics with padding bits so that cmpxchg on them works, which means that we'll likely need to do the zero-init song and dance for empty atomic structs anyways (and I think we shouldn't special-case this behavior to C++20 because prior standards are just broken).

This patch therefore makes a minor change to r176658 "Promote atomic type sizes up to a power of two": if the width of the atomic's value type is 0, just use 1 byte for width and alignment.

This fixes an assertion:

(NumBits >= MIN_INT_BITS && "bitwidth too small"), function get, file ../lib/IR/Type.cpp, line 241.

It seems like this has run into other assertions before (namely the unreachable Kind check in ImpCastExprToType), but I haven't reproduced that issue with tip-of-tree.

rdar://problem/39678063

Diff Detail

Event Timeline

jfb created this revision.May 8 2018, 5:18 PM
rjmccall added inline comments.May 8 2018, 6:41 PM
lib/AST/ASTContext.cpp
1965

Alignment, unlike size, is definitely never 0. I think you should leave the original alignment in place; it's a weird case, but we honor over-aligned empty types in a bunch of other places.

jfb updated this revision to Diff 145858.May 8 2018, 8:48 PM
  • Assert on zero alignment, instead of making it always byte-aligned.
jfb marked an inline comment as done.May 8 2018, 8:49 PM
jfb added inline comments.
lib/AST/ASTContext.cpp
1965

Yeah that makes total sense. I turned it to an assert.

rjmccall accepted this revision.May 8 2018, 8:50 PM

LGTM.

This revision is now accepted and ready to land.May 8 2018, 8:50 PM
This revision was automatically updated to reflect the committed changes.
jfb marked an inline comment as done.