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

[Chandler Carruth]

On Tue, Aug 28, 2012 at 3:52 PM, John McCall <rjmccall at apple.com> wrote:

> On Aug 28, 2012, at 3:18 PM, Chandler Carruth wrote:
> > I've added an explanatory comment. Hopefully won't get stale.
> >
> > Any other comments? I'd love to get this into the tree as we're seeing
> actual races in the wild due to it....
>
> If you would like a fast review, perhaps a more minimal change would
> have been in order. We're not going to rush into a total rewrite of
> bitfield
> IR-generation, especially not one that introduces a host of new assumptions
> and dependencies on a very general but poorly-specified IR feature that
> has not previously been used in code with binary compatibility
> requirements.
>

So, first off, sorry for being in a hurry. I'm not trying to get a rushed
review, just hoping to get review sooner rather than later. =D Anyways, I
would *really* like thorough reviews of this. I'm very aware and sensitive
to the importance of this code. In fact, that's why I'm working on it.

Also, I too would have preferred a more minimal change. None seemed readily
available when both I and Richard Smith were looking at this in detail.

The primary idea we had was to bound the strided accesses to the size of
the bitfield storage rather than just to the containing struct. This should
be sufficient, but it has quite a few downsides in my mind:
1) The easy way to implement it regresses performance as in many cases it
would force us to load one byte at a time
2) The efficient way to implement it would require significantly more code
to synthesize peeling off the leading and trailing regions. This is
essentially the same code that is in LLVM to legalize these types, and it
seemed a waste to just duplicate the logic.
3) Most of this would still require one of the more significant changes in
this patch -- establishing the range of the storage for the bitfields by
laying them out in groups.

My suspicion is this would have either returned us to the byte-by-byte
model (unacceptable performance) or been an even larger patch with more
duplication from LLVM.

I also happened to be hacking on SROA recently, and that pass was in fact
forming similar IR structures you're worried about: arbitrarily sized
integers with bit insertion and extraction as a "vector" of "bits". I'm not
claiming that it produced the exact same structures or that there aren't
bugs here. I would be more than happy to provide extra testing and try to
flush out any crufty bits of LLVM here. I'm just saying I don't think this
pattern is *completely* unused...

Anyways, does even the vague approach seem good? Should I work on a
different strategy? Are there particular tests that I might be able to run?
Any of this would be helpful feedback for me here.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120828/b1d8e723/attachment.html>