On Tue, Sep 11, 2012 at 11:33 AM, John McCall <span dir="ltr"><<a href="mailto:rjmccall@apple.com" target="_blank">rjmccall@apple.com</a>></span> wrote:<br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5">On Sep 11, 2012, at 11:07 AM, Chandler Carruth wrote:<br>
> On Fri, Aug 31, 2012 at 1:01 PM, Eli Friedman <<a href="mailto:eli.friedman@gmail.com">eli.friedman@gmail.com</a>> wrote:<br>
> On Mon, Aug 27, 2012 at 10:33 AM, John McCall <<a href="mailto:rjmccall@apple.com">rjmccall@apple.com</a>> wrote:<br>
> > On Aug 27, 2012, at 9:55 AM, Matthieu Monrocq wrote:<br>
> ><br>
> > On Mon, Aug 27, 2012 at 11:17 AM, Chandler Carruth <<a href="mailto:chandlerc@google.com">chandlerc@google.com</a>><br>
> > wrote:<br>
> >><br>
> >> Hello,<br>
> >><br>
> >> I've attached a *draft* patch that is a re-working of our record layout<br>
> >> and IR generation for bitfields. The previous strategy inherently introduced<br>
> >> widened stores (and loads) that violate the C++11 memory model, causing<br>
> >> PR13691. The old system was also rather elaborate without much<br>
> >> justification. My patch shrinks the code significantly.<br>
> >><br>
> >> LLVM supports arbitrary sized integers, and bitfields *really are* the<br>
> >> equivalent of a bag-of-bits or integer-vector that these integers support<br>
> >> well. SRoA has been forming such integers in LLVM for some time. The various<br>
> >> targets are in a good position to split and simplify these operations into<br>
> >> the efficient representation... and when they fail, we should fix the<br>
> >> target. So this patch essentially interpets a run of contiguous bitfields as<br>
> >> a single large integer, and emits loads stores and bit operations<br>
> >> appropriate for extracting bits from within it. This leaves any valid load<br>
> >> widening to LLVM passes that are more target aware and also aware of<br>
> >> instrumentation such as ASan or TSan that might be invalidated by load<br>
> >> widening.<br>
> >><br>
> >> This has a few advantages:<br>
> >> - Very dense IR, we can now pre-compute the GEP when building the LValue<br>
> >> for a bitfield, and that will be the only GEP needed. Also, the math to<br>
> >> extract the value is simpler with this model.<br>
> >> - Maximal exposure to the optimizer of coalescing opportunities across<br>
> >> bitfields. The SRoA of the IR produces will end up with a single SSA value<br>
> >> for the collection of bits in the bitfield.<br>
> >><br>
> >> It also has a two specific disadvantages in its current form, but I think<br>
> >> I can address them eventually:<br>
> >> - It doesn't take advantage of unused bits, which can be assumed to be<br>
> >> all-zero and save some masking.<br>
> >> - It also doesn't take advantage of padding following the bitfield to<br>
> >> widen stores safely<br>
> >><br>
> >> The last one is very tricky to do correctly. We have to know that the<br>
> >> object is POD-for-layout. I want to address this separately w/ some good<br>
> >> test cases.<br>
> >><br>
> ><br>
> > Just a reminder here, a derived class may use the padding of its base and<br>
> > stuff things there.<br>
> ><br>
> ><br>
> > Only if it's not POD, which I assume is why Chandler brought that up.<br>
><br>
> Another nasty case I just thought of:<br>
><br>
> struct x { int i : 24; };<br>
> struct y { int i : 24; char j; };<br>
> union z {<br>
>   struct x x;<br>
>   struct y y;<br>
> };<br>
> union z a;<br>
> void f(int i) {<br>
>   a.x.i = i;<br>
> }<br>
> void g(char j) {<br>
>   a.y.j = j<br>
> }<br>
><br>
> The two writes are to separate memory locations. :)<br>
><br>
> Wait, hold on... I'm deeply confused. Maybe because I don't know how C11 unions work?<br>
><br>
> 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?<br>
<br>
</div></div>I agree that this isn't a problem, but the analysis is a bit more complicated;<br>
it hinges on the fact that it's okay to *read* from an inactive union member<br>
under the common-prefix exception, but it's not okay to *write* to it.  The<br>
same analysis applies in both standards:<br></blockquote><div><br></div><div>Is this still OK if the extra union member is volatile? Chandler and I have discussed this, and it's far from clear that it would be. (In particular, we can conceive of a seemingly-reasonable case where the union sits on an MMIO port, and only the fourth byte has volatile semantics.)</div>
</div>