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

[Alexandre Oliva <aoliva at redhat dot com>]

On Mar  3, 2009, Richard Guenther <rguenther@suse.de> wrote:

> On Mon, 2 Mar 2009, Alexandre Oliva wrote:
>> On Mar  2, 2009, Richard Guenther <rguenther@suse.de> 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.

This is the bit that was supposed to implement it:

-   if ((bit & ~alchk)
-       || (HOST_WIDE_INT)size != tree_low_cst (DECL_SIZE (var), 1))
-     {
-       block->replacement = fold_build3 (BIT_FIELD_REF,
- 					TREE_TYPE (block->element), var,
- 					bitsize_int (size),
- 					bitsize_int (bit & ~alchk));
-     }

See how it the replacement is a bit-field of the scalar variable
starting at BIT (masking off the multiples of the "word" size), rather
than at the LSB?  Later on, the actual fields are further extracted from
block->replacement.

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

Are you sure?  Did you look at the tree IR, or at the actual output?

The point of exposing the shifts to the tree IR is precisely so that
they can be optimized away, when possible, as is often the case for such
bit-fields that aren't accessed directly.  The intermediate temps are
most often optimized away.

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

I'm pretty sure it's related.  If the code previously expected a 32-bit
word-sized integer to use bits 23..31 from it, and it was turned into a
9-bit integer, I can imagine why you'd get surprising results ;-)

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

Remember that we're not talking about a single field at that point,
we're trying to lump multiple fields that are not accessed individually
into a single variable.

If there is a scalar mode that can access all such fields at the same
time, we'll use that.  But if there isn't, accessing the separate
"words", shifting stuff about and putting it together is probably more
efficiently done in the individual (rare) boundary-crossing field,
shifting and masking narrower integer types, than bundling the field
along with multiple other fields into a single much-wider word, for
which we don't even have a scalar mode.

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

Please compare the actual output code and see that it doesn't
significantly undo the improvements achieved by the original
bit-field-bundling patch.  I suspect it will, because of the added
unnecessary shifting.

-- 
Alexandre Oliva           http://www.lsd.ic.unicamp.br/~oliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist      Red Hat Brazil Compiler Engineer