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

[Matthieu Monrocq]

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. So you would need to be sure of the dynamic type of the
object to widen the stores safely, or risk overwriting fields stuffed there
by the derived class.

-- Matthieu


>
> 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120827/ec7342f0/attachment.html>