Re: Need advice with store_bit_field and its call chain

[Jim Wilson <wilson at specifixinc dot com>]

On Mon, 2003-10-27 at 19:24, Zack Weinberg wrote:
> Would you mind making and testing a patch to use GET_MODE_SIZE
> instead, then?

Well, OK, I decided I cared enough to try to write a patch.  However,
I'm left wondering if you understand the consequences of your
GET_MODE_BITSIZE changes.  I think you have created hundreds of latent
bugs, and I expect your cooperation in getting them fixed.

I found a good example of the kinds of problems created while fixing
these bit field problems.  The if statement in store_bit_field that I am
fixing has
	MEM_ALIGN (op0) % GET_MODE_SIZE (fieldmode) == 0
This used to be correct, but is now wrong because of your patch. 
Alignment depends on the size not the precision, so this fails for IA-64
XFmode.  I included a fix for this one place in my patch because it was
easy, but I haven't tried fixing all of the others.  There are likely
many other places in the compiler that have the same bug, or similar
ones.

I wrote a patch to fix the bit field issues correctly, but I am not
happy with the result.  I need to replace
	GET_MODE_BITSIZE (fieldmode)
with
	GET_MODE_SIZE (fieldmode) * BITS_PER_UNIT
because using GET_MODE_BITSIZE here is no longer correct. 
Unfortunately, GET_MODE_SIZE returns an unsigned char.  BITS_PER_UNIT is
a signed int constant.  The result of the multiply is thus a signed
integer, and I get compile time warnings for comparing signed and
unsigned values.  Thus I had to instead write
	GET_MODE_SIZE (fieldmode) * (unsigned) BITS_PER_UNIT
which is pretty inelegant.

I think the right way to solve this is to rename the new
GET_MODE_BITSIZE stuff to GET_MODE_PRECISION as I suggested earlier, and
then add a GET_MODE_BITSIZE macro that works the same way as the old
one.  This would let me write an elegant patch to fix this problem.  It
also will help make the compiler faster, by avoiding the need for a
multiply here.   As the old saying goes, don't compute what you can
precompute.  This primarily means the mode_bitsize array in insn-modes.c
gets renamed to mode_precision.  And we add a mode_bitsize array that
contains mode_size*BITS_PER_UNIT, which was the original meaning of
GET_MODE_BITSIZE.

However, changing the meaning of GET_MODE_BITSIZE twice in a short
period of time may itself cause problems.  Perhaps we should poison
GET_MODE_BITSIZE, and come up with a new name for the replacement macro
to avoid problems?

It we do keep the old name, then there is the question of which uses of
GET_MODE_BITSIZE get changed to use GET_MODE_PRECISION.  I suggested
changing all of them earlier, but not all of them should be changed.  I
doubt that you want to spend the time necessary to determine which ones
should be GET_MODE_PRECISION and which ones should be GET_MODE_BITSIZE
though, but that would be the ideal solution.  A compromise solution
would be change them all to GET_MODE_PRECISION, and then start changing
them back to GET_MODE_BITSIZE as we examine them.  Maybe you can help
with this?

We can't change uses of GET_MODE_BITSIZE that exist outside our
repository.  Hence the suggestion above that maybe it should be
poisoned.

I've attached the patch I wrote.  This was tested with an x86-linux
bootstrap and make check.  Also, I tested the two bit-field testcases
with a cross-compiler to ia64-linux.  I haven't check this in yet in the
hopes that maybe I can add a more elegant solution as described above.
-- 
Jim Wilson, GNU Tools Support, http://www.SpecifixInc.com

Attachment: tmp.patch.bitsize
Description: Text document