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

[John McCall]

On Aug 28, 2012, at 4:34 PM, Chandler Carruth wrote:
> 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.

Why not simply bound it to the offset of the next non-bitfield field (or, if there is none, the data size of the type)?  Is that challenging for some reason I'm not understanding?

> 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.

I asked Daniel to make a detailed review, since he wrote the current implementation and did a lot of investigation for it.

The things that worry me most are:

(1) How well-specified and -tested are the paths for non-power-of-2 integer types in LLVM?  Do we actually promise that, say, storing to an i17 will not touch the fourth byte?  Do we provide reasonably portable behavior across targets, or is this an instance of the x86 backend having received a lot of attention and everything else being a festering morass of latent issues?  I know that SROA's behavior has exposed bugs before (IIRC with the occasional response of making SROA less aggressive), not least of which because SROA's behavior doesn't really kick in a lot.

(2) How limiting is this going to be for bitfields on "weird" platforms?  Do we get anything useful from this change on big-endian targets?  Will we find ourselves completely reimplementing it to support MS-style packed bitfields? etc.

When I surveyed a few other people for their first impressions (including Chris and Daniel), there was also a lot of concern that the backend will often not be able to widen these loads and stores because the high-level information simply isn't there.  In fact, it sounds an awful lot like, in order to satisfy the memory model when widening loads and stores, the compiler will need to prove the lack of races on the memory.  That is basically equivalent to "only in trivial examples".

John.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120828/c50983a6/attachment.html>