Patchwork PR 55438: Incorrect use of BIGGEST_ALIGNMENT

login
register
mail settings
Submitter Richard Sandiford
Date Nov. 24, 2012, 6:33 p.m.
Message ID <871ufi7svh.fsf@talisman.default>
Download mbox | patch
Permalink /patch/201474/
State New
Headers show

Comments

Richard Sandiford - Nov. 24, 2012, 6:33 p.m.
In http://gcc.gnu.org/ml/gcc-patches/2012-11/msg00257.html I said:

  get_best_mode has various checks to decide what counts as an acceptable
  bitfield mode.  It actually has two copies of them, with slightly different
  alignment checks:

    MIN (unit, BIGGEST_ALIGNMENT) > align

  vs.

    unit <= MIN (align, BIGGEST_ALIGNMENT)

  The second looks more correct, since we can't necessarily guarantee
  larger alignments than BIGGEST_ALIGNMENT in all cases.

PR 55438 shows why I was wrong.  BIGGEST_ALIGNMENT is not (as I thought)
the biggest supported alignment, but the:

  Biggest alignment that any data type can require on this machine, in
  bits.  Note that this is not the biggest alignment that is supported,
  just the biggest alignment that, when violated, may cause a fault.

So it's perfectly possible for BIGGEST_ALIGNMENT to be 8 on 32-bit machines.

As things stand, my change causes get_best_mode to reject anything larger
than a byte for that combination, which causes infinite recursion between
store_fixed_bit_field and store_split_bit_field.

There are two problems.  First:

  if (!bitregion_end_)
    {
      /* We can assume that any aligned chunk of ALIGN_ bits that overlaps
	 the bitfield is mapped and won't trap.  */
      bitregion_end_ = bitpos + bitsize + align_ - 1;
      bitregion_end_ -= bitregion_end_ % align_ + 1;
    }

is too conservative if the aligment is capped to BIGGEST_ALIGNMENT.
We should be able to guarantee that word-aligned memory is mapped
in at least word-sized chunks.  (If you think now is the time to
add a MIN_PAGE_SIZE macro, perhaps with MAX (BITS_PER_WORD,
BIGGEST_ALIGNMENT) as its default, I can do that instead.)

Also, in cases like these, the supposedly conservative:

	 && GET_MODE_BITSIZE (mode) <= align

doesn't preserve the cap in the original:

    MIN (unit, BIGGEST_ALIGNMENT) > align

Fixed by using GET_MODE_ALIGNMENT instead.

Tested on x86_64-linux-gnu, powerpc64-linux-gnu and cris-elf.
I checked that the output for a set of gcc .ii files didn't
change for the first two.  OK to install?

Richard


gcc/
	PR middle-end/55438
	* stor-layout.c (bit_field_mode_iterator::bit_field_mode_iterator):
	Don't limit ALIGN_.  Assume that memory is mapped in chunks of at
	least word size, regardless of BIGGEST_ALIGNMENT.
	(get_best_mode): Use GET_MODE_ALIGNMENT.
Eric Botcazou - Nov. 26, 2012, 5:41 p.m.
> In http://gcc.gnu.org/ml/gcc-patches/2012-11/msg00257.html I said:
> 
>   get_best_mode has various checks to decide what counts as an acceptable
>   bitfield mode.  It actually has two copies of them, with slightly
> different alignment checks:
> 
>     MIN (unit, BIGGEST_ALIGNMENT) > align
> 
>   vs.
> 
>     unit <= MIN (align, BIGGEST_ALIGNMENT)
> 
>   The second looks more correct, since we can't necessarily guarantee
>   larger alignments than BIGGEST_ALIGNMENT in all cases.
> 
> PR 55438 shows why I was wrong.  BIGGEST_ALIGNMENT is not (as I thought)
> the biggest supported alignment, but the:
> 
>   Biggest alignment that any data type can require on this machine, in
>   bits.  Note that this is not the biggest alignment that is supported,
>   just the biggest alignment that, when violated, may cause a fault.
> 
> So it's perfectly possible for BIGGEST_ALIGNMENT to be 8 on 32-bit machines.

That means that the Sequent check was flawed, doesn't it?  It also seems that 
the entire business of alignment with size comparisons is dubious.

> As things stand, my change causes get_best_mode to reject anything larger
> than a byte for that combination, which causes infinite recursion between
> store_fixed_bit_field and store_split_bit_field.
> 
> There are two problems.  First:
> 
>   if (!bitregion_end_)
>     {
>       /* We can assume that any aligned chunk of ALIGN_ bits that overlaps
> 	 the bitfield is mapped and won't trap.  */
>       bitregion_end_ = bitpos + bitsize + align_ - 1;
>       bitregion_end_ -= bitregion_end_ % align_ + 1;
>     }
> 
> is too conservative if the aligment is capped to BIGGEST_ALIGNMENT.
> We should be able to guarantee that word-aligned memory is mapped
> in at least word-sized chunks.  (If you think now is the time to
> add a MIN_PAGE_SIZE macro, perhaps with MAX (BITS_PER_WORD,
> BIGGEST_ALIGNMENT) as its default, I can do that instead.)

So this will fix the Sequent check with a conservative cap of BITS_PER_WORD.
That's reasonable, I think.

> Also, in cases like these, the supposedly conservative:
> 
> 	 && GET_MODE_BITSIZE (mode) <= align
> 
> doesn't preserve the cap in the original:
> 
>     MIN (unit, BIGGEST_ALIGNMENT) > align
> 
> Fixed by using GET_MODE_ALIGNMENT instead.

Note that the comment just above needs to be adjusted then.  What about the 
similar check in next_mode?

      /* Stop if the mode requires too much alignment.  */
      if (unit > align_ && SLOW_UNALIGNED_ACCESS (mode_, align_))
	break;

It seems to me that we should change it as well.
Richard Sandiford - Nov. 26, 2012, 6:59 p.m.
Eric Botcazou <ebotcazou@adacore.com> writes:
>> In http://gcc.gnu.org/ml/gcc-patches/2012-11/msg00257.html I said:
>> 
>>   get_best_mode has various checks to decide what counts as an acceptable
>>   bitfield mode.  It actually has two copies of them, with slightly
>> different alignment checks:
>> 
>>     MIN (unit, BIGGEST_ALIGNMENT) > align
>> 
>>   vs.
>> 
>>     unit <= MIN (align, BIGGEST_ALIGNMENT)
>> 
>>   The second looks more correct, since we can't necessarily guarantee
>>   larger alignments than BIGGEST_ALIGNMENT in all cases.
>> 
>> PR 55438 shows why I was wrong.  BIGGEST_ALIGNMENT is not (as I thought)
>> the biggest supported alignment, but the:
>> 
>>   Biggest alignment that any data type can require on this machine, in
>>   bits.  Note that this is not the biggest alignment that is supported,
>>   just the biggest alignment that, when violated, may cause a fault.
>> 
>> So it's perfectly possible for BIGGEST_ALIGNMENT to be 8 on 32-bit machines.
>
> That means that the Sequent check was flawed, doesn't it?  It also seems that 
> the entire business of alignment with size comparisons is dubious.

Yeah, I suppose the Sequent test didn't really have any effect on
BIGGEST_ALIGNMENT == BITS_PER_UNIT targets.

>> Also, in cases like these, the supposedly conservative:
>> 
>> 	 && GET_MODE_BITSIZE (mode) <= align
>> 
>> doesn't preserve the cap in the original:
>> 
>>     MIN (unit, BIGGEST_ALIGNMENT) > align
>> 
>> Fixed by using GET_MODE_ALIGNMENT instead.
>
> Note that the comment just above needs to be adjusted then.

OK, how about:

	 /* ??? For historical reasons, reject modes that would normally
	    receive greater alignment, even if unaligned accesses are
	    acceptable.  This has both advantages and disadvantages.

> What about the similar check in next_mode?
>
>       /* Stop if the mode requires too much alignment.  */
>       if (unit > align_ && SLOW_UNALIGNED_ACCESS (mode_, align_))
> 	break;
>
> It seems to me that we should change it as well.

I think here we really do want unit (i.e. the GET_MODE_BITSIZE).
We're dividing the bitfield into unit-sized chunks and want to know
whether those chunks are aligned or not.  If they aren't aligned,
we need to know whether unaligned accesses are OK.

Richard
Eric Botcazou - Nov. 26, 2012, 9:38 p.m.
> OK, how about:
> 
> 	 /* ??? For historical reasons, reject modes that would normally
> 	    receive greater alignment, even if unaligned accesses are
> 	    acceptable.  This has both advantages and disadvantages.

Fine with me.

> I think here we really do want unit (i.e. the GET_MODE_BITSIZE).
> We're dividing the bitfield into unit-sized chunks and want to know
> whether those chunks are aligned or not.  If they aren't aligned,
> we need to know whether unaligned accesses are OK.

We could conceivably have an aligned mode with bitsize 32 and alignment 16 
(i.e. STRICT_ALIGNMENT 1 and BIGGEST_ALIGNMENT 16), meaning that we can use 
16-bit aligned 32-bit chunks in this mode.  Why would the bitsize matter 
(assuming that it's a multiple of the alignment)?
Andreas Schwab - Nov. 26, 2012, 11:40 p.m.
Eric Botcazou <ebotcazou@adacore.com> writes:

> We could conceivably have an aligned mode with bitsize 32 and alignment 16 
> (i.e. STRICT_ALIGNMENT 1 and BIGGEST_ALIGNMENT 16), meaning that we can use 
> 16-bit aligned 32-bit chunks in this mode.

That's actually what m68k traditionally had.

Andreas.

Patch

Index: gcc/stor-layout.c
===================================================================
--- gcc/stor-layout.c	2012-11-24 10:24:46.000000000 +0000
+++ gcc/stor-layout.c	2012-11-24 10:33:19.011617853 +0000
@@ -2643,15 +2643,17 @@  fixup_unsigned_type (tree type)
 			   unsigned int align, bool volatilep)
 : mode_ (GET_CLASS_NARROWEST_MODE (MODE_INT)), bitsize_ (bitsize),
   bitpos_ (bitpos), bitregion_start_ (bitregion_start),
-  bitregion_end_ (bitregion_end), align_ (MIN (align, BIGGEST_ALIGNMENT)),
+  bitregion_end_ (bitregion_end), align_ (align),
   volatilep_ (volatilep), count_ (0)
 {
   if (!bitregion_end_)
     {
-      /* We can assume that any aligned chunk of ALIGN_ bits that overlaps
+      /* We can assume that any aligned chunk of UNITS bits that overlaps
 	 the bitfield is mapped and won't trap.  */
-      bitregion_end_ = bitpos + bitsize + align_ - 1;
-      bitregion_end_ -= bitregion_end_ % align_ + 1;
+      unsigned HOST_WIDE_INT units = MIN (align, MAX (BIGGEST_ALIGNMENT,
+						      BITS_PER_WORD));
+      bitregion_end_ = bitpos + bitsize + units - 1;
+      bitregion_end_ -= bitregion_end_ % units + 1;
     }
 }
 
@@ -2808,7 +2810,7 @@  get_best_mode (int bitsize, int bitpos,
 	    causes store_bit_field to keep a 128-bit memory reference,
 	    so that the final bitfield reference still has a MEM_EXPR
 	    and MEM_OFFSET.  */
-	 && GET_MODE_BITSIZE (mode) <= align
+	 && GET_MODE_ALIGNMENT (mode) <= align
 	 && (largest_mode == VOIDmode
 	     || GET_MODE_SIZE (mode) <= GET_MODE_SIZE (largest_mode)))
     {