Need advice with store_bit_field and its call chain
Jim Wilson
wilson@specifixinc.com
Fri Oct 31 07:52:00 GMT 2003
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
-------------- next part --------------
2003-10-29 James E Wilson <wilson@specifixinc.com>
* expmed.c (store_bit_field): New local fieldmode_bitsize. Use instead
of GET_MODE_BITSIZE (fieldmode). Change size test back to equality
tests.
(extract_bit_field): For full register case, change size test back to
equality test. Use GET_MODE_SIZE instead of GET_MODE_BITSIZE.
Index: expmed.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/expmed.c,v
retrieving revision 1.145
diff -p -r1.145 expmed.c
*** expmed.c 25 Oct 2003 23:14:14 -0000 1.145
--- expmed.c 30 Oct 2003 06:23:10 -0000
*************** store_bit_field (rtx str_rtx, unsigned H
*** 287,292 ****
--- 287,296 ----
= (GET_CODE (str_rtx) == MEM) ? BITS_PER_UNIT : BITS_PER_WORD;
unsigned HOST_WIDE_INT offset = bitnum / unit;
unsigned HOST_WIDE_INT bitpos = bitnum % unit;
+ /* We can't use GET_MODE_BITSIZE here, because we need the size not the
+ precision. This matters for XFmode on IA-64. */
+ unsigned HOST_WIDE_INT fieldmode_bitsize
+ = GET_MODE_SIZE (fieldmode) * (unsigned) BITS_PER_UNIT;
rtx op0 = str_rtx;
int byte_offset;
*************** store_bit_field (rtx str_rtx, unsigned H
*** 326,349 ****
If the target is memory, storing any naturally aligned field can be
done with a simple store. For targets that support fast unaligned
! memory, any naturally sized, unit aligned field can be done directly.
!
! It's okay if the requested bitsize is greater than fieldmode's
! bitsize; that just means the mode has padding bits. */
byte_offset = (bitnum % BITS_PER_WORD) / BITS_PER_UNIT
+ (offset * UNITS_PER_WORD);
if (bitpos == 0
! && GET_MODE_BITSIZE (fieldmode) != 0
! && bitsize >= GET_MODE_BITSIZE (fieldmode)
&& (GET_CODE (op0) != MEM
? ((GET_MODE_SIZE (fieldmode) >= UNITS_PER_WORD
|| GET_MODE_SIZE (GET_MODE (op0)) == GET_MODE_SIZE (fieldmode))
&& byte_offset % GET_MODE_SIZE (fieldmode) == 0)
: (! SLOW_UNALIGNED_ACCESS (fieldmode, MEM_ALIGN (op0))
|| (offset * BITS_PER_UNIT % bitsize == 0
! && MEM_ALIGN (op0) % GET_MODE_BITSIZE (fieldmode) == 0))))
{
if (GET_MODE (op0) != fieldmode)
{
--- 330,349 ----
If the target is memory, storing any naturally aligned field can be
done with a simple store. For targets that support fast unaligned
! memory, any naturally sized, unit aligned field can be done directly. */
byte_offset = (bitnum % BITS_PER_WORD) / BITS_PER_UNIT
+ (offset * UNITS_PER_WORD);
if (bitpos == 0
! && bitsize == fieldmode_bitsize
&& (GET_CODE (op0) != MEM
? ((GET_MODE_SIZE (fieldmode) >= UNITS_PER_WORD
|| GET_MODE_SIZE (GET_MODE (op0)) == GET_MODE_SIZE (fieldmode))
&& byte_offset % GET_MODE_SIZE (fieldmode) == 0)
: (! SLOW_UNALIGNED_ACCESS (fieldmode, MEM_ALIGN (op0))
|| (offset * BITS_PER_UNIT % bitsize == 0
! && MEM_ALIGN (op0) % fieldmode_bitsize == 0))))
{
if (GET_MODE (op0) != fieldmode)
{
*************** store_bit_field (rtx str_rtx, unsigned H
*** 407,413 ****
if (GET_CODE (op0) != MEM
&& (BYTES_BIG_ENDIAN ? bitpos + bitsize == unit : bitpos == 0)
! && bitsize == GET_MODE_BITSIZE (fieldmode)
&& (movstrict_optab->handlers[(int) fieldmode].insn_code
!= CODE_FOR_nothing))
{
--- 407,413 ----
if (GET_CODE (op0) != MEM
&& (BYTES_BIG_ENDIAN ? bitpos + bitsize == unit : bitpos == 0)
! && bitsize == fieldmode_bitsize
&& (movstrict_optab->handlers[(int) fieldmode].insn_code
!= CODE_FOR_nothing))
{
*************** extract_bit_field (rtx str_rtx, unsigned
*** 1033,1044 ****
if (GET_CODE (op0) == REG
&& mode == GET_MODE (op0)
&& bitnum == 0
! && GET_MODE_BITSIZE (GET_MODE (op0)) != 0
! && bitsize >= GET_MODE_BITSIZE (GET_MODE (op0)))
{
! /* We're trying to extract a full register from itself.
! (If the requested bitsize is greater than the bitsize of op0,
! that just means op0's mode has padding bits.) */
return op0;
}
--- 1033,1043 ----
if (GET_CODE (op0) == REG
&& mode == GET_MODE (op0)
&& bitnum == 0
! /* We can't use GET_MODE_BITSIZE here, because we need the size not the
! precision. This matters for XFmode on IA-64. */
! && bitsize == (GET_MODE_SIZE (mode) * (unsigned) BITS_PER_UNIT))
{
! /* We're trying to extract a full register from itself. */
return op0;
}
More information about the Gcc
mailing list