<div class="gmail_extra"><div class="gmail_quote">On Fri, Aug 31, 2012 at 1:01 PM, Eli Friedman <span dir="ltr"><<a href="mailto:eli.friedman@gmail.com" target="_blank" class="cremed">eli.friedman@gmail.com</a>></span> wrote:<br>
<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 Mon, Aug 27, 2012 at 10:33 AM, John McCall <<a href="mailto:rjmccall@apple.com" class="cremed">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" class="cremed">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>
</div></div>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></blockquote><div><br></div><div>Wait, hold on... I'm deeply confused. Maybe because I don't know how C11 unions work?</div><div><br></div><div>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?</div>
<div><br></div><div>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'.</div><div><br></div><div>If you can clarify how C11 says I should disable this, I'm happy to adjust.</div>
</div></div>