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

[Sean Silva]

-class CGBitFieldInfo {
-public:
-  /// Descriptor for a single component of a bit-field access. The entire
-  /// bit-field is constituted of a bitwise OR of all of the individual
-  /// components.
-  ///
-  /// Each component describes an accessed value, which is how the component
-  /// should be transferred to/from memory, and a target placement,
which is how
-  /// that component fits into the constituted bit-field. The pseudo-IR for a
-  /// load is:
-  ///
-  ///   %0 = gep %base, 0, FieldIndex
-  ///   %1 = gep (i8*) %0, FieldByteOffset
-  ///   %2 = (i(AccessWidth) *) %1
-  ///   %3 = load %2, align AccessAlignment
-  ///   %4 = shr %3, FieldBitStart
-  ///
-  /// and the composed bit-field is formed as the boolean OR of all accesses,
-  /// masked to TargetBitWidth bits and shifted to TargetBitOffset.

Could you beef up the 2-liner comment you replaced this nice comment
with to include the pseudo-IR for a bitfield access like this comment
does? I think that's a very valuable piece of documentation to have.

--Sean Silva

On Mon, Aug 27, 2012 at 8:20 PM, Chandler Carruth <chandlerc at google.com> wrote:
> Updated patch.
>
> Now passes all regression tests. Tests for the PR in question have been
> added.
>
> I've implemented some really gross logic in the ObjCRuntime part of IR-Gen.
> Hope it's OK... =/
>
> I'd love to get some feedback on this and commit it soon. I'll be
> bootstrapping and running it through the test suite when I can.
>
>
> On Mon, Aug 27, 2012 at 2: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.
>>
>>
>> I hope to send up a cleaned up patch with test cases and results from a
>> bootstrap and maybe test suite run tomorrow, but wanted to give folks an
>> opportunity to comment on the basic strategy.
>>
>> Somewhat terrifyingly, *all current regression tests pass*. =/
>
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>