Re: [0/8] Add optabs alternatives for insv, extv and extzv

[Richard Sandiford <rdsandiford@googlemail.com>]

Ramana Radhakrishnan <ramana.gcc@googlemail.com> writes:
> On Tue, Nov 27, 2012 at 8:22 PM, Richard Sandiford
> <rdsandiford@googlemail.com> wrote:
>> Ramana Radhakrishnan <ramrad01@arm.com> writes:
>>>> Tested on x86_64-linux-gnu, powerpc64-linux-gnu and mipsisa64-elf.
>>>> Also tested by making sure that there were no changes in assembly
>>>> output for a set of gcc .ii files.  On the other hand, the -march=octeon
>>>> output for a set of mips64-linux-gnu gcc .ii files showed the optimisation
>>>> kicking in as hoped.
>>>
>>> This sequence of patches caused a regression in
>>> gcc.target/arm/volatile-bitfields-1.c . I haven't reviewed the patch
>>> stack in great detail yet, but it looks like this sequence of patches
>>> doesn't take the ARM volatile bitfields into account. (193600 is fine,
>>> 193606 is not).
>>
>> Looks like I was wrong to drop the conditions from patch 5.
>> Please could you try the attached?
>
> Fixes volatile-bitfields-1.c but appears to break volatile-bitfields-4.c :( .

This is because the structure we are given is:

    (mem/v/j:SI (reg/v/f:SI 110 [ t ]) [3 t_2(D)->a+0 S1 A32])

i.e. a 1-byte SImode reference.  The strange size comes from the
set_mem_attributes_minus_bitpos code that was mentioned earlier
in the series, where we set the size based on the type even if
it doesn't match the mode.

The original rules for forcing strict-volatile bitfields from memory
to registers were different (or at least written in a different way)
between store_bit_field_1 -- where we force to a register in an attempt
to use register insvs -- and store_fixed_bit_field -- where we use the
fallback shifts and masks -- even though logically I thought they'd be
the same.  In store_bit_field_1 the mode was chosen like this:

      /* Get the mode to use for inserting into this field.  If OP0 is
	 BLKmode, get the smallest mode consistent with the alignment. If
	 OP0 is a non-BLKmode object that is no wider than OP_MODE, use its
	 mode. Otherwise, use the smallest mode containing the field.  */

      if (GET_MODE (op0) == BLKmode
	  || (op_mode != MAX_MACHINE_MODE
	      && GET_MODE_SIZE (GET_MODE (op0)) > GET_MODE_SIZE (op_mode)))
	bestmode = get_best_mode (bitsize, bitnum, MEM_ALIGN (op0),
				  (op_mode == MAX_MACHINE_MODE
				   ? VOIDmode : op_mode),
				  MEM_VOLATILE_P (op0));
      else
	bestmode = GET_MODE (op0);

      if (bestmode != VOIDmode
	  && GET_MODE_SIZE (bestmode) >= GET_MODE_SIZE (fieldmode)
	  && !(SLOW_UNALIGNED_ACCESS (bestmode, MEM_ALIGN (op0))
	       && GET_MODE_BITSIZE (bestmode) > MEM_ALIGN (op0)))
	{

i.e. no explicit test of flag_strict_volatile_bitfields.
In store_fixed_bit_field we had:

      mode = GET_MODE (op0);
      if (GET_MODE_BITSIZE (mode) == 0
	  || GET_MODE_BITSIZE (mode) > GET_MODE_BITSIZE (word_mode))
	mode = word_mode;

      if (MEM_VOLATILE_P (op0)
          && GET_MODE_BITSIZE (GET_MODE (op0)) > 0
	  && flag_strict_volatile_bitfields > 0)
	mode = GET_MODE (op0);
      else
	mode = get_best_mode (bitsize, bitpos + offset * BITS_PER_UNIT,
			      MEM_ALIGN (op0), mode, MEM_VOLATILE_P (op0));

The same thing applied to extract_bit_field_1 and extract_fixed_bit_field,
with the latter using:

      if (MEM_VOLATILE_P (op0)
	  && flag_strict_volatile_bitfields > 0)
	{
	  if (GET_MODE_BITSIZE (GET_MODE (op0)) > 0)
	    mode = GET_MODE (op0);
	  else if (target && GET_MODE_BITSIZE (GET_MODE (target)) > 0)
	    mode = GET_MODE (target);
	  else
	    mode = tmode;
	}
      else
	mode = get_best_mode (bitsize, bitpos + offset * BITS_PER_UNIT,
			      MEM_ALIGN (op0), word_mode, MEM_VOLATILE_P (op0));

This patch seems to fix the volatile bitfield tests, both with the
default arm-linux-gnueabi (which already works) and with
-mcpu=cortex-a8.  Could you give it a proper test?

Richard


gcc/
	* expmed.c (adjust_bit_field_mem_for_reg): Add handling of volatile
	bitfields.

Index: gcc/expmed.c
===================================================================
--- gcc/expmed.c	(revision 193881)
+++ gcc/expmed.c	(working copy)
@@ -372,6 +372,17 @@
 				bitregion_end, MEM_ALIGN (op0),
 				MEM_VOLATILE_P (op0));
   enum machine_mode best_mode;
+  if (MEM_VOLATILE_P (op0) && flag_strict_volatile_bitfields > 0)
+    {
+      unsigned int size = GET_MODE_BITSIZE (GET_MODE (op0));
+      if (size > 0)
+	while (iter.next_mode (&best_mode))
+	  if (GET_MODE_BITSIZE (best_mode) == size)
+	    return narrow_bit_field_mem (op0, best_mode, bitsize, bitnum,
+					 new_bitnum);
+      return NULL_RTX;
+    }
+
   if (iter.next_mode (&best_mode))
     {
       /* We can use a memory in BEST_MODE.  See whether this is true for