Remove word_mode hack for split bitfields

Remove word_mode hack for split bitfields

[Richard Sandiford]

This patch is effectively reverting a change from 1994.  The reason
I think it's a hack is that store_bit_field_1 is creating a subreg
reference to one word of a field even though it has already proven that
the field spills into the following word.  We then rely on the special
SUBREG handling in store_split_bit_field to ignore the extent of op0 and
look inside the SUBREG_REG regardless.  I don't see any reason why we can't
pass the original op0 to store_split_bit_field instead.

Tested on aarch64-linux-gnu and x86_64-linux-gnu.  OK to install?

Thanks,
Richard


gcc/
	* expmed.c (store_bit_field_1): Do not restrict a multiword op0
	to one word if the field is known to overlap other words.
	(extract_bit_field_1): Likewise.
	(store_split_bit_field): Remove compensating code.
	(extract_split_bit_field): Likewise.

diff --git a/gcc/expmed.c b/gcc/expmed.c
index ec968da..6645a53 100644
--- a/gcc/expmed.c
+++ b/gcc/expmed.c
@@ -967,11 +967,7 @@ store_bit_field_1 (rtx str_rtx, unsigned HOST_WIDE_INT bitsize,
      If the region spans two words, defer to store_split_bit_field.  */
   if (!MEM_P (op0) && GET_MODE_SIZE (GET_MODE (op0)) > UNITS_PER_WORD)
     {
-      op0 = simplify_gen_subreg (word_mode, op0, GET_MODE (op0),
-				 bitnum / BITS_PER_WORD * UNITS_PER_WORD);
-      gcc_assert (op0);
-      bitnum %= BITS_PER_WORD;
-      if (bitnum + bitsize > BITS_PER_WORD)
+      if (bitnum % BITS_PER_WORD + bitsize > BITS_PER_WORD)
 	{
 	  if (!fallback_p)
 	    return false;
@@ -980,6 +976,10 @@ store_bit_field_1 (rtx str_rtx, unsigned HOST_WIDE_INT bitsize,
 				 bitregion_end, value, reverse);
 	  return true;
 	}
+      op0 = simplify_gen_subreg (word_mode, op0, GET_MODE (op0),
+				 bitnum / BITS_PER_WORD * UNITS_PER_WORD);
+      gcc_assert (op0);
+      bitnum %= BITS_PER_WORD;
     }
 
   /* From here on we can assume that the field to be stored in fits
@@ -1383,25 +1383,8 @@ store_split_bit_field (rtx op0, unsigned HOST_WIDE_INT bitsize,
 					    bitsdone, NULL_RTX, 1, false);
 	}
 
-      /* If OP0 is a register, then handle OFFSET here.
-
-	 When handling multiword bitfields, extract_bit_field may pass
-	 down a word_mode SUBREG of a larger REG for a bitfield that actually
-	 crosses a word boundary.  Thus, for a SUBREG, we must find
-	 the current word starting from the base register.  */
-      if (GET_CODE (op0) == SUBREG)
-	{
-	  int word_offset = (SUBREG_BYTE (op0) / UNITS_PER_WORD)
-			    + (offset * unit / BITS_PER_WORD);
-	  machine_mode sub_mode = GET_MODE (SUBREG_REG (op0));
-	  if (sub_mode != BLKmode && GET_MODE_SIZE (sub_mode) < UNITS_PER_WORD)
-	    word = word_offset ? const0_rtx : op0;
-	  else
-	    word = operand_subword_force (SUBREG_REG (op0), word_offset,
-					  GET_MODE (SUBREG_REG (op0)));
-	  offset &= BITS_PER_WORD / unit - 1;
-	}
-      else if (REG_P (op0))
+      /* If OP0 is a register, then handle OFFSET here.  */
+      if (SUBREG_P (op0) || REG_P (op0))
 	{
 	  machine_mode op0_mode = GET_MODE (op0);
 	  if (op0_mode != BLKmode && GET_MODE_SIZE (op0_mode) < UNITS_PER_WORD)
@@ -1787,10 +1770,7 @@ extract_bit_field_1 (rtx str_rtx, unsigned HOST_WIDE_INT bitsize,
      If the region spans two words, defer to extract_split_bit_field.  */
   if (!MEM_P (op0) && GET_MODE_SIZE (GET_MODE (op0)) > UNITS_PER_WORD)
     {
-      op0 = simplify_gen_subreg (word_mode, op0, GET_MODE (op0),
-				 bitnum / BITS_PER_WORD * UNITS_PER_WORD);
-      bitnum %= BITS_PER_WORD;
-      if (bitnum + bitsize > BITS_PER_WORD)
+      if (bitnum % BITS_PER_WORD + bitsize > BITS_PER_WORD)
 	{
 	  if (!fallback_p)
 	    return NULL_RTX;
@@ -1798,6 +1778,9 @@ extract_bit_field_1 (rtx str_rtx, unsigned HOST_WIDE_INT bitsize,
 					    reverse);
 	  return convert_extracted_bit_field (target, mode, tmode, unsignedp);
 	}
+      op0 = simplify_gen_subreg (word_mode, op0, GET_MODE (op0),
+				 bitnum / BITS_PER_WORD * UNITS_PER_WORD);
+      bitnum %= BITS_PER_WORD;
     }
 
   /* From here on we know the desired field is smaller than a word.
@@ -2109,20 +2092,8 @@ extract_split_bit_field (rtx op0, unsigned HOST_WIDE_INT bitsize,
       thissize = MIN (bitsize - bitsdone, BITS_PER_WORD);
       thissize = MIN (thissize, unit - thispos);
 
-      /* If OP0 is a register, then handle OFFSET here.
-
-	 When handling multiword bitfields, extract_bit_field may pass
-	 down a word_mode SUBREG of a larger REG for a bitfield that actually
-	 crosses a word boundary.  Thus, for a SUBREG, we must find
-	 the current word starting from the base register.  */
-      if (GET_CODE (op0) == SUBREG)
-	{
-	  int word_offset = (SUBREG_BYTE (op0) / UNITS_PER_WORD) + offset;
-	  word = operand_subword_force (SUBREG_REG (op0), word_offset,
-					GET_MODE (SUBREG_REG (op0)));
-	  offset = 0;
-	}
-      else if (REG_P (op0))
+      /* If OP0 is a register, then handle OFFSET here.  */
+      if (SUBREG_P (op0) || REG_P (op0))
 	{
 	  word = operand_subword_force (op0, offset, GET_MODE (op0));
 	  offset = 0;

Re: Remove word_mode hack for split bitfields

[Bernd Schmidt]

On 05/26/2016 04:36 PM, Richard Sandiford wrote:
> This patch is effectively reverting a change from 1994.  The reason
> I think it's a hack is that store_bit_field_1 is creating a subreg
> reference to one word of a field even though it has already proven that
> the field spills into the following word.  We then rely on the special
> SUBREG handling in store_split_bit_field to ignore the extent of op0 and
> look inside the SUBREG_REG regardless.  I don't see any reason why we can't
> pass the original op0 to store_split_bit_field instead.
>
> Tested on aarch64-linux-gnu and x86_64-linux-gnu.  OK to install?

Any observable effects on code generation?


Bernd

Re: Remove word_mode hack for split bitfields

[Richard Sandiford]

Bernd Schmidt <bschmidt@redhat.com> writes:
> On 05/26/2016 04:36 PM, Richard Sandiford wrote:
>> This patch is effectively reverting a change from 1994.  The reason
>> I think it's a hack is that store_bit_field_1 is creating a subreg
>> reference to one word of a field even though it has already proven that
>> the field spills into the following word.  We then rely on the special
>> SUBREG handling in store_split_bit_field to ignore the extent of op0 and
>> look inside the SUBREG_REG regardless.  I don't see any reason why we can't
>> pass the original op0 to store_split_bit_field instead.
>>
>> Tested on aarch64-linux-gnu and x86_64-linux-gnu.  OK to install?
>
> Any observable effects on code generation?

Not that I can see.  I tried compiling gcc.dg, g++.dg and gcc.c-torture
before and after the patch on:

    aarch64-linux-gnueabi alpha-linux-gnu arm-linux-gnueabi
    arm-linux-gnueabihf avr-elf bfin-elf c6x-elf cr16-elf cris-elf
    epiphany-elf fr30-elf frv-linux-gnu ft32-elf h8300-elf
    ia64-linux-gnu iq2000-elf lm32-elf m32c-elf m32r-elf m68k-linux-gnu
    mcore-elf microblaze-elf mips-linux-gnu mmix mn10300-elf moxie-rtems
    msp430-elf nds32le-elf hppa64-hp-hpux11.23 nios2-linux-gnu
    nvptx-none powerpc-linux-gnu powerpc-eabispe rl78-elf rx-elf
    s390-linux-gnu sh-linux-gnu sparc-linux-gnu spu-elf tilegx-elf
    tilepro-elf xstormy16-elf v850-elf vax-netbsdelf visium-elf
    xtensa-elf x86_64-darwin

The only asm differences (apart from timestamps) were a handful of tests
for avr-elf:

    gcc.c-torture/execute/pr53645.s
    gcc.c-torture/execute/pr65427.s
    gcc.dg/pr57233.s
    gcc.dg/torture/pr30665-2.s
    gcc.dg/tree-ssa/pr23391.s

and c6x-elf:

    gcc.c-torture/compile/pr42956.s
    gcc.dg/compat/vector-1a_y.s
    gcc.dg/compat/vector-1b_y.s
    gcc.dg/torture/vshuf-v8sf.s

In all these cases the rtl seems to be the same before reload,
but differences in addresses(?) cause different, but equivalent, RA.
None of the LRA targets seem to be affected.

Thanks,
Richard

Re: Remove word_mode hack for split bitfields

[Bernd Schmidt]

On 05/26/2016 04:36 PM, Richard Sandiford wrote:
> This patch is effectively reverting a change from 1994.  The reason
> I think it's a hack is that store_bit_field_1 is creating a subreg
> reference to one word of a field even though it has already proven that
> the field spills into the following word.  We then rely on the special
> SUBREG handling in store_split_bit_field to ignore the extent of op0 and
> look inside the SUBREG_REG regardless.  I don't see any reason why we can't
> pass the original op0 to store_split_bit_field instead.
>
> Tested on aarch64-linux-gnu and x86_64-linux-gnu.  OK to install?

I think it's OK. Ideally we'd know why the 1994 change was made, but 
that's beyond my archaeological abiliy. The code looked very different 
at the time and probably changed over the years to make this 
simplification possible.


Bernd