On Mon, 2 Mar 2009, Alexandre Oliva wrote:
> On Mar 2, 2009, Richard Guenther <rguenther@xxxxxxx> wrote:
> > (basically the scalar replacement covers all and only the extracted
> > part, so sub-setting it needs to be relative to this part, not the
> > original struct).
> Hmm... This doesn't sound quite right.
> Perhaps I misremember, but my plan was that bitfield-sets (i.e.,
> multiple fields scalarized into a single integral variable, rather than
> each getting its own variable) were to retain the same bit position as
> the "words" in the struct they were extracted from.
Hm, this certainly isn't what was implemented.
> So, if we created a bitfield scalar to cover say bits 23 to 31 of a
> 32-bit word, the relevant bits in this variable ought to be in the
> 23..31 range, rather than in the 0..8 range. The idea being that this
> would save on shifting: such scalar bitfields would generally be used to
> extract and reinsert into the original struct, so masking ought to
But shifting is created anyways and for accessing parts of that
field you still need it anyways. With the patch we create less
shifting on the testcase btw.
> The wider words also makes room for efficiently storing non-contiguous
> fields that aren't accessed by themselves to the point of deserving
> their own separate variables in the same scalarized variable. However,
> I never got to implement that.
> I wonder if this problem is a consequence of your change to use
> non-standard integer types narrowed down to the exact used bit width,
> rather than the "word" width, which I advised against a while ago.
> > Alexandre - what is this alchk variable about?
> Alignment check: the idea is to use it to determine when we cross a
> "word" boundary, given the known alignment of the struct and/or of the
> field. We don't want to access "words" that would cross alignment
> boundaries, but at the same time we don't want to pick a wasteful width
> for the variable.
> So, ALCHK is the mask that we use to test that beginning and end of a
> field are within the same ALIGN boundary ("word"), that two fields are
> in the same "word", and, when negated, to compute the offsets within the
> chosen word.
Huh. I don't see why we don't want to access alignment-crossing
words (or choose a scalar replacement for an alignment-crossing field).
But well, I'm not going to touch that code more than necessary ;)
> > Does this look sane?
> It looks like a correct match for the change in the BF scalar type,
> although I believe it will generate less efficient code on some
> platforms, because AFAICT you're introducing more bit extraction
> (shifting) where plain masking would have been enough.
> Personally, I think it would make far more sense to go back to the wider
> integral types. I don't see the value of hiding from the middle-high
> end the fact that we have more bits available in the variable, rather
> than using them to improve code generation.
I don't think we want to do that for 4.4 now. And for 4.5 we
have a completely new SRA implementation anyway.
> > Btw, I had to remove the tree.c instrumentation from the patch as
> > it triggers during Ada tools build where we end up building
> > BIT_FIELD_REF <char, bool:1, 8, 0> from SRA. But that seems to
> > be an unrelated goof.
> I think this construct may come up if the mode of the bool:1 type, or of
> the field itself, is wide enough for the 8 requested bits.
I'm going to install the patch if there are no more complaints.