gcc-patches@gcc.gnu.org
[Top] [All Lists]

Re: [PATCH] Fix PR39339, wrong-code from SRA with bitfields

Subject: Re: [PATCH] Fix PR39339, wrong-code from SRA with bitfields
From: Richard Guenther
Date: Tue, 3 Mar 2009 12:11:48 +0100 CET
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
> suffice.

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.

Maybe.

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

I'm going to install the patch if there are no more complaints.

Thanks,
Richard.

<Prev in Thread] Current Thread [Next in Thread>