| Message ID | 87fwhjcdh9.fsf@firetop.home |
|---|---|
| State | New |
| Headers | show |
> Yeah, I don't think constants are any different here. One fix might be > to use simplify_expand_binop instead of extract_bit_field, as per the > patch below. The patch also restricts the shifting to forward walks, > as discussed in the PR trail. This looks fine to me, please apply (with a few words added to the comment, e.g. "...doesn't have full wordsize and we transfer in memory order..."). > The point of r8007 seems to be that WORDS_BIG_ENDIAN doesn't apply to > BLKmode fieldmodes, but there must surely be some endianness involved > somewhere, given that we're extracting words from a multiword value? I think that BLKmode objects are treated as left-justified in memory order, i.e. left-justified on big-endian and right-justified on little-endian. > So if possible, I'd prefer to revert the original patch for this PR > and instead adjust the "backwards" test. I'm just not sure whether > it should be "WORDS_BIG_ENDIAN" (reverting r8007, which was obviously > applied for a reason) or something like: > > (fieldmode == BLKmode ? BYTES_BIG_ENDIAN : WORDS_BIG_ENDIAN) This seems to be a little risky for stage 3 though.
Eric Botcazou <ebotcazou@adacore.com> writes: >> Yeah, I don't think constants are any different here. One fix might be >> to use simplify_expand_binop instead of extract_bit_field, as per the >> patch below. The patch also restricts the shifting to forward walks, >> as discussed in the PR trail. > > This looks fine to me, please apply (with a few words added to the comment, > e.g. "...doesn't have full wordsize and we transfer in memory order..."). > >> The point of r8007 seems to be that WORDS_BIG_ENDIAN doesn't apply to >> BLKmode fieldmodes, but there must surely be some endianness involved >> somewhere, given that we're extracting words from a multiword value? > > I think that BLKmode objects are treated as left-justified in memory order, > i.e. left-justified on big-endian and right-justified on little-endian. > >> So if possible, I'd prefer to revert the original patch for this PR >> and instead adjust the "backwards" test. I'm just not sure whether >> it should be "WORDS_BIG_ENDIAN" (reverting r8007, which was obviously >> applied for a reason) or something like: >> >> (fieldmode == BLKmode ? BYTES_BIG_ENDIAN : WORDS_BIG_ENDIAN) > > This seems to be a little risky for stage 3 though. What's the main potential problem you see? The backwards condition: (fieldmode == BLKmode ? BYTES_BIG_ENDIAN : WORDS_BIG_ENDIAN) should apply the same bit-for-bit mapping between source and target as the patch applies. The only difference should be in the order that the reads and writes happen. But does that matter? If we were worried about something like volatile MEMs here, I'd expect a check for that, since volatile MEMs can be something like doubleword as well as BLKmode. Richard
> What's the main potential problem you see? The backwards condition: > > (fieldmode == BLKmode ? BYTES_BIG_ENDIAN : WORDS_BIG_ENDIAN) > > should apply the same bit-for-bit mapping between source and target > as the patch applies. Not if the objects don't have the same size. In Ada we have BLKmode bitfields, i.e. fields whose type has BLKmode and whose DECL_SIZE is smaller than the TYPE_SIZE of the type; in this case, we want to drop the rightmost bytes in memory order, whatever the endianness.
Eric Botcazou <ebotcazou@adacore.com> writes: >> What's the main potential problem you see? The backwards condition: >> >> (fieldmode == BLKmode ? BYTES_BIG_ENDIAN : WORDS_BIG_ENDIAN) >> >> should apply the same bit-for-bit mapping between source and target >> as the patch applies. > > Not if the objects don't have the same size. Ugh. > In Ada we have BLKmode bitfields, i.e. fields whose type has BLKmode > and whose DECL_SIZE is smaller than the TYPE_SIZE of the type; in this > case, we want to drop the rightmost bytes in memory order, whatever > the endianness. OK, I see how the two are different now, but I still don't get why the current version is right. If we say words are 16 bits for simplicity, and we're storing VALUE == 0x0001_2345 into a 3-byte BLKmode bitfield, surely we want to store 0x01_2345 rather than 0x00_0123? It seems odd for fieldmode (which I thought was a property of the target) to affect the interpretation of VALUE (the source). Richard
> OK, I see how the two are different now, but I still don't get why the > current version is right. If we say words are 16 bits for simplicity, > and we're storing VALUE == 0x0001_2345 into a 3-byte BLKmode bitfield, > surely we want to store 0x01_2345 rather than 0x00_0123? Why? If you want this semantics, you need to use a bitfield with integer mode. In Ada, the following case is frequent: you have a record type with BLKmode and an alignment that causes it to padded (because the size is always a multiple of the alignment). You have a field in another record type whose type is the first record type but with a representation clause that prescribes the size without the padding. The Ada compiler must accept that. So the field is made a BLKmode bitfield, with DECL_SIZE (size w/o padding) smaller than TYPE_SIZE (size w/ padding). When you're assigning a value to the field, you drop the rightmost bytes in memory order, which are junk since padding.
Eric Botcazou <ebotcazou@adacore.com> writes: >> OK, I see how the two are different now, but I still don't get why the >> current version is right. If we say words are 16 bits for simplicity, >> and we're storing VALUE == 0x0001_2345 into a 3-byte BLKmode bitfield, >> surely we want to store 0x01_2345 rather than 0x00_0123? > > Why? If you want this semantics, you need to use a bitfield with integer mode It's the interpretation and mode of VALUE that I'm worried about. Doesn't FIELDMODE describe STR_RTX (the stored bitfield) instead? If so, it seems odd that we're using a property of STR_RTX to determine the interpretation/justification of VALUE. That is, we're using FIELDMODE before, rather than after: /* This is the mode we must force value to, so that there will be enough subwords to extract. Note that fieldmode will often (always?) be VOIDmode, because that is what store_field uses to indicate that this is a bit field, but passing VOIDmode to operand_subword_force is not allowed. */ fieldmode = GET_MODE (value); if (fieldmode == VOIDmode) fieldmode = smallest_mode_for_size (nwords * BITS_PER_WORD, MODE_INT); > In Ada, the following case is frequent: you have a record type with BLKmode and > an alignment that causes it to padded (because the size is always a multiple > of the alignment). You have a field in another record type whose type is the > first record type but with a representation clause that prescribes the size > without the padding. The Ada compiler must accept that. So the field is made > a BLKmode bitfield, with DECL_SIZE (size w/o padding) smaller than TYPE_SIZE > (size w/ padding). When you're assigning a value to the field, you drop the > rightmost bytes in memory order, which are junk since padding. In this sort of assignment, what does VALUE look like? Does it need to be BLKmode? Or can it have an integer mode? (I assume the former, since the latter seems contrary to what you said about integer modes above.) Must VALUE be a memory? Could we assert: gcc_assert ((fieldmode == BLKmode) == (GET_MODE (value) == BLKmode)); gcc_assert (fieldmode != BLKmode || MEM_P (value)); at the beginning of store_bit_field_1? Since Andreas was fixing C++ bugs, I assume the same situation occurs there. What does it look like in a C++ context? Richard
On 11/23/2011 03:47 PM, Richard Sandiford wrote: > Since Andreas was fixing C++ bugs, I assume the same situation occurs there. > What does it look like in a C++ context? http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50325#c4 value was (subreg:DI (reg:TI 70) 8) and fieldmode == BLKmode -Andreas-
On 21/11/11 08:02, Eric Botcazou wrote:
> This seems to be a little risky for stage 3 though.
Being in stage 3 shouldn't stop us trying to fix bugs in the compiler:
we're not in the final run-up to a release yet (that could still be five
months away if history is anything to go by). It should just be a
clamp-down on major new functionality.
R.
> Being in stage 3 shouldn't stop us trying to fix bugs in the compiler: > we're not in the final run-up to a release yet (that could still be five > months away if history is anything to go by). It should just be a > clamp-down on major new functionality. But Richard's proposed change wasn't really a bugfix (he also proposed a bugfix which should be applied of course) but rather a cleanup. As the ongoing story shows, this is a delicate area so, if we are to clean things up here at this stage, the level of testing should at least be adjusted.
> It's the interpretation and mode of VALUE that I'm worried about. > Doesn't FIELDMODE describe STR_RTX (the stored bitfield) instead? Yes, FIELDMODE is the mode of the target field. > If so, it seems odd that we're using a property of STR_RTX to determine > the interpretation/justification of VALUE. That is, we're using > FIELDMODE before, rather than after: > > /* This is the mode we must force value to, so that there will be > enough subwords to extract. Note that fieldmode will often (always?) be > VOIDmode, because that is what store_field uses to indicate that this is a > bit field, but passing VOIDmode to operand_subword_force > is not allowed. */ > fieldmode = GET_MODE (value); > if (fieldmode == VOIDmode) > fieldmode = smallest_mode_for_size (nwords * BITS_PER_WORD, MODE_INT); First of all, reusing FIELDMODE here is clearly very confusing, this should be VALUEMODE instead. OK, I understand the concern. But what if VALUE has VOIDmode and the field BLKmode? In this case, VALUEMODE may be integral and using it to determine the justification will be wrong. > In this sort of assignment, what does VALUE look like? Does it need to be > BLKmode? Or can it have an integer mode? (I assume the former, since the > latter seems contrary to what you said about integer modes above.) I think it must be BLKmode or VOIDmode. > Must VALUE be a memory? Or a constant probably. > Could we assert: > > gcc_assert ((fieldmode == BLKmode) == (GET_MODE (value) == BLKmode)); > gcc_assert (fieldmode != BLKmode || MEM_P (value)); > > at the beginning of store_bit_field_1? We could indeed try, modulo constants I think.
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50325#c4 > > value was (subreg:DI (reg:TI 70) 8) Not the original VALUE though, since it is 12-byte wide.
Eric Botcazou <ebotcazou@adacore.com> writes: >> If so, it seems odd that we're using a property of STR_RTX to determine >> the interpretation/justification of VALUE. That is, we're using >> FIELDMODE before, rather than after: >> >> /* This is the mode we must force value to, so that there will be >> enough subwords to extract. Note that fieldmode will often (always?) be >> VOIDmode, because that is what store_field uses to indicate that this is a >> bit field, but passing VOIDmode to operand_subword_force >> is not allowed. */ >> fieldmode = GET_MODE (value); >> if (fieldmode == VOIDmode) >> fieldmode = smallest_mode_for_size (nwords * BITS_PER_WORD, MODE_INT); > > First of all, reusing FIELDMODE here is clearly very confusing, this > should be VALUEMODE instead. > > OK, I understand the concern. But what if VALUE has VOIDmode and the field > BLKmode? In this case, VALUEMODE may be integral and using it to determine > the justification will be wrong. Ah, so in the BLKmode case, VALUE can still be a nonzero const_int? What are the justification rules in that case? Naively, I'd have expected a store of (const_int 1) into a BLKmode field to set one of the bits to 1, but it sounds from the above like it depends on endianness: for little-endian targets it always sets a bit, but for big-endian targets it only sets a bit if the field width is a multiple of UNITS_PER_WORD. >> In this sort of assignment, what does VALUE look like? Does it need to be >> BLKmode? Or can it have an integer mode? (I assume the former, since the >> latter seems contrary to what you said about integer modes above.) > > I think it must be BLKmode or VOIDmode. > >> Must VALUE be a memory? > > Or a constant probably. > >> Could we assert: >> >> gcc_assert ((fieldmode == BLKmode) == (GET_MODE (value) == BLKmode)); >> gcc_assert (fieldmode != BLKmode || MEM_P (value)); >> >> at the beginning of store_bit_field_1? > > We could indeed try, modulo constants I think. OK, thanks, that sounds better. If we can iron out what the nonzero constant case means then I'd be happy with that. Richard
> Ah, so in the BLKmode case, VALUE can still be a nonzero const_int? Yes, I'd think so. > What are the justification rules in that case? Naively, I'd have > expected a store of (const_int 1) into a BLKmode field to set one > of the bits to 1, but it sounds from the above like it depends on > endianness: for little-endian targets it always sets a bit, but for > big-endian targets it only sets a bit if the field width is a multiple > of UNITS_PER_WORD. For example, in Ada, the following program: with Text_IO; use Text_IO; with Unchecked_Conversion; procedure P is type Arr is new String(1..3); function Conv is new Unchecked_Conversion (Integer, Arr); type R is record A : Arr; end record; My_R : R; begin My_R.A := Conv (16#41424344#); Put (String(My_R.A)); end; outputs DCB on little-endian and ABC on big-endian.
Index: gcc/optabs.h =================================================================== --- gcc/optabs.h 2011-11-19 16:44:44.000000000 +0000 +++ gcc/optabs.h 2011-11-19 16:44:58.000000000 +0000 @@ -869,6 +869,10 @@ extern rtx expand_ternary_op (enum machi extern rtx expand_binop (enum machine_mode, optab, rtx, rtx, rtx, int, enum optab_methods); +extern rtx simplify_expand_binop (enum machine_mode mode, optab binoptab, + rtx op0, rtx op1, rtx target, int unsignedp, + enum optab_methods methods); + extern bool force_expand_binop (enum machine_mode, optab, rtx, rtx, rtx, int, enum optab_methods); Index: gcc/optabs.c =================================================================== --- gcc/optabs.c 2011-11-19 16:44:31.000000000 +0000 +++ gcc/optabs.c 2011-11-19 16:44:34.000000000 +0000 @@ -671,7 +671,7 @@ expand_ternary_op (enum machine_mode mod calculated at compile time. The arguments and return value are otherwise the same as for expand_binop. */ -static rtx +rtx simplify_expand_binop (enum machine_mode mode, optab binoptab, rtx op0, rtx op1, rtx target, int unsignedp, enum optab_methods methods) Index: gcc/expmed.c =================================================================== --- gcc/expmed.c 2011-11-19 16:43:26.000000000 +0000 +++ gcc/expmed.c 2011-11-20 09:22:24.000000000 +0000 @@ -564,10 +564,13 @@ store_bit_field_1 (rtx str_rtx, unsigned /* If the remaining chunk doesn't have full wordsize we have to make sure that for big endian machines the higher order bits are used. */ - if (new_bitsize < BITS_PER_WORD && BYTES_BIG_ENDIAN) - value_word = extract_bit_field (value_word, new_bitsize, 0, - true, false, NULL_RTX, - BLKmode, word_mode); + if (new_bitsize < BITS_PER_WORD && BYTES_BIG_ENDIAN && !backwards) + value_word = simplify_expand_binop (word_mode, lshr_optab, + value_word, + GEN_INT (BITS_PER_WORD + - new_bitsize), + NULL_RTX, true, + OPTAB_LIB_WIDEN); if (!store_bit_field_1 (op0, new_bitsize, bitnum + bit_offset,