[cfe-commits] PATCH: Large re-working of bitfield IR-gen, and a fix for PR13691

[Eli Friedman]

On Mon, Aug 27, 2012 at 10:33 AM, John McCall <rjmccall at apple.com> wrote:
> On Aug 27, 2012, at 9:55 AM, Matthieu Monrocq wrote:
>
> On Mon, Aug 27, 2012 at 11:17 AM, Chandler Carruth <chandlerc at google.com>
> 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. :)

-Eli