| Message ID | 871ufi7svh.fsf@talisman.default |
|---|---|
| State | New |
| Headers | show |
> 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.
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
> 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)?
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.
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))) {
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.