On Fri, Aug 31, 2012 at 1:01 PM, Eli Friedman <[email protected]>wrote:
> On Mon, Aug 27, 2012 at 10:33 AM, John McCall <[email protected]> wrote: > > On Aug 27, 2012, at 9:55 AM, Matthieu Monrocq wrote: > > > > On Mon, Aug 27, 2012 at 11:17 AM, Chandler Carruth <[email protected] > > > > wrote: > >> > >> Hello, > >> > >> I've attached a *draft* patch that is a re-working of our record layout > >> and IR generation for bitfields. The previous strategy inherently > introduced > >> widened stores (and loads) that violate the C++11 memory model, causing > >> PR13691. The old system was also rather elaborate without much > >> justification. My patch shrinks the code significantly. > >> > >> LLVM supports arbitrary sized integers, and bitfields *really are* the > >> equivalent of a bag-of-bits or integer-vector that these integers > support > >> well. SRoA has been forming such integers in LLVM for some time. The > various > >> targets are in a good position to split and simplify these operations > into > >> the efficient representation... and when they fail, we should fix the > >> target. So this patch essentially interpets a run of contiguous > bitfields as > >> a single large integer, and emits loads stores and bit operations > >> appropriate for extracting bits from within it. This leaves any valid > load > >> widening to LLVM passes that are more target aware and also aware of > >> instrumentation such as ASan or TSan that might be invalidated by load > >> widening. > >> > >> This has a few advantages: > >> - Very dense IR, we can now pre-compute the GEP when building the LValue > >> for a bitfield, and that will be the only GEP needed. Also, the math to > >> extract the value is simpler with this model. > >> - Maximal exposure to the optimizer of coalescing opportunities across > >> bitfields. The SRoA of the IR produces will end up with a single SSA > value > >> for the collection of bits in the bitfield. > >> > >> It also has a two specific disadvantages in its current form, but I > think > >> I can address them eventually: > >> - It doesn't take advantage of unused bits, which can be assumed to be > >> all-zero and save some masking. > >> - It also doesn't take advantage of padding following the bitfield to > >> widen stores safely > >> > >> The last one is very tricky to do correctly. We have to know that the > >> object is POD-for-layout. I want to address this separately w/ some good > >> test cases. > >> > > > > Just a reminder here, a derived class may use the padding of its base and > > stuff things there. > > > > > > Only if it's not POD, which I assume is why Chandler brought that up. > > Another nasty case I just thought of: > > struct x { int i : 24; }; > struct y { int i : 24; char j; }; > union z { > struct x x; > struct y y; > }; > union z a; > void f(int i) { > a.x.i = i; > } > void g(char j) { > a.y.j = j > } > > The two writes are to separate memory locations. :) > Wait, hold on... I'm deeply confused. Maybe because I don't know how C11 unions work? With C++11, only one side of the union is allowed to be active, and so I don't think they are separate memory locations? Before I added the optimization to widen into known-padding-bits, this was handled correctly, but now we take advantage of the padding bits in 'x'. If you can clarify how C11 says I should disable this, I'm happy to adjust.
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
