This is the mail archive of the gcc@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Need advice with store_bit_field and its call chain


Jim Wilson <wilson@specifixinc.com> writes:

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

Of course I will help fix bugs.  However, my time is limited for the
next week and then I'm completely unavailable for the following two
weeks. 

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

You meant to say GET_MODE_BITSIZE here, yes?

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

GET_MODE_SIZE should have a cast to unsigned int embedded in it.  The
tables are logically unsigned int; they're just declared as unsigned
char to save space.  (I didn't introduce this btw.)

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

I mostly agree with this, but I'm not convinced it is appropriate to
add a whole new table.  It will make the code generated for
ADJUST_MODE_SIZE more complicated, and multiplication by constant 8/16/32
will get optimized to a left shift; some architectures may even be
able to do the load and the shift in one instruction.

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

GET_MODE_SIZE_IN_BITS ?  Perhaps a bit long winded.

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

I expect I have time to make a start at this on Sunday, and can
continue working on it through the week.  If you and others can assist
as well, we ought to be able to finish it by the time I disappear on
the 9th.

zw


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]