Hi, On Thu, 5 Dec 2013 15:10:51, Richard Biener wrote: > > On Thu, Dec 5, 2013 at 1:27 PM, Bernd Edlinger > <bernd.edlin...@hotmail.de> wrote: >> Hi Richard, >> >> I had just an idea how to solve that recursion problem without completely >> ignoring the >> memory mode. I hope you are gonna like it. >> >> This time I even added a comment :-) > > Ehm, ... > > + /* If MODE has no size i.e. BLKmode or is larger than WORD_MODE > + or BITREGION_END is used we can use WORD_MODE as upper limit. > + However, if the field is too unaligned to be fetched with a single > + access, we also have to use WORD_MODE. This can happen in Ada. */ > if (GET_MODE_BITSIZE (mode) == 0 > - || GET_MODE_BITSIZE (mode)> GET_MODE_BITSIZE (word_mode)) > + || GET_MODE_BITSIZE (mode)> GET_MODE_BITSIZE (word_mode) > + || bitregion_end != 0 > + || bitnum % BITS_PER_UNIT + bitsize> GET_MODE_SIZE (mode) > + || (STRICT_ALIGNMENT > + && bitnum % GET_MODE_ALIGNMENT (mode) + bitsize > +> GET_MODE_BITSIZE (mode))) > mode = word_mode; > > If the field is unaligned how does choosing a larger mode help? We should > always be able to use multiple accesses with a smaller mode in this case. > > So - I'd buy the || bitregion_end != 0 thing but the rest ...? So, ok if > that alone fixes the bug. Note that when bitregion_end == 0 the > access should be limited by the mode we pass to get_best_mode. > > Btw, it should be always possible to return QImode here, so I wonder > how enlarging the mode fixes the underlying issue (maybe the bug > is really elsewhere?) >
This comment in store_split_bit_field explains everything: /* THISSIZE must not overrun a word boundary. Otherwise, store_fixed_bit_field will call us again, and we will mutually recurse forever. */ thissize = MIN (bitsize - bitsdone, BITS_PER_WORD); thissize = MIN (thissize, unit - thispos); This comment was added in 1993. At that time the code in store_fixed_bit_field looked like this: mode = get_best_mode (bitsize, bitnum, bitregion_start, bitregion_end, MEM_ALIGN (op0), word_mode, MEM_VOLATILE_P (op0)); if (mode == VOIDmode) { /* The only way this should occur is if the field spans word boundaries. */ store_split_bit_field (op0, bitsize, bitnum, bitregion_start, bitregion_end, value); return; } And in 2001 that was changed to this: mode = GET_MODE (op0); if (GET_MODE_BITSIZE (mode) == 0 || GET_MODE_BITSIZE (mode)> GET_MODE_BITSIZE (word_mode)) mode = word_mode; mode = get_best_mode (bitsize, bitnum, bitregion_start, bitregion_end, MEM_ALIGN (op0), mode, MEM_VOLATILE_P (op0)); Which broke the symmetry, and invalidated the above comment. Therefore we have again a recursion problem. BUT ONLY, because the Ada front-end comes here, and tries to write a QImode value at bitpos=11, bitsize=8 GET_MODE(op0) = QImode, which is obviously not a Byte, but at least 2 Bytes, or a word, or something different.... So, Yes that fixes the recursion, and makes the test case pass. Boot-Strap on x86_64-linux-gnu OK. Regression-test still running... Note: I've noticed, that in the previous version of the change-log I had the line " (store_fixed_bit_field_1): New function." duplicated. The patch it-self is the same, but I nevertheless attach it again. OK for trunk? Thanks Bernd. > Thanks, > Richard. > >> Ok for trunk after boot-strap and regression-testing? >> >> >> Bernd. >> >> On Tue, 3 Dec 2013 12:23:11, Richard Biener wrote: >>> >>> On Tue, Dec 3, 2013 at 12:01 PM, Bernd Edlinger >>> <bernd.edlin...@hotmail.de> wrote: >>>> Hi, >>>> >>>> This is my proposal for ulimately getting rid of the nasty >>>> store_fixed_bit_field recursion. >>>> >>>> IMHO, the root of the recursion trouble is here: >>>> >>>> @@ -1007,12 +1013,8 @@ store_fixed_bit_field (rtx op0, unsigned >>>> >>>> if (MEM_P (op0)) >>>> { >>>> mode = GET_MODE (op0); >>>> if (GET_MODE_BITSIZE (mode) == 0 >>>> || GET_MODE_BITSIZE (mode)> GET_MODE_BITSIZE (word_mode)) >>>> mode = word_mode; >>>> mode = get_best_mode (bitsize, bitnum, bitregion_start, bitregion_end, >>>> MEM_ALIGN (op0), mode, MEM_VOLATILE_P (op0)); >>>> >>>> >>>> But, because now we always have bitregion_start and bitregion_end to limit >>>> the access size, it is no longer necessary to restrict the largest mode, >>>> that >>>> get_best_mode may return. >>> >>> Note that this is not true, as get_bit_range itself may end up giving >>> up with bitregion_start = bitregion_end = 0. But maybe this is not >>> what you are saying here? That is, does a >>> >>> gcc_assert (bitregion_start != 0 || bitregion_end != 0); >>> >>> before the get_best_mode call work for you? >>> >>>> This patch is very similar to the previous patch, which split up the >>>> extract_fixed_bit_field, >>>> >>>> This time, I just split up store_fixed_bit_field and use >>>> store_fixed_bit_field_1 to force the >>>> strict-volatile-bitfield mode it necessary, and let get_best_mode find a >>>> mode, that is >>>> can be used to access the field, which is no longer impacted by the memory >>>> context's selected >>>> mode in this case. >>>> >>>> I tried this patch with an ARM-Cross compiler and a large eCos >>>> application, to see if anything >>>> changes in the generated code with this patch, but 2 MB of code stays >>>> binary the same, >>>> that's a good sign. >>>> >>>> I added the new Ada test case, and the test case from PR59134, which does >>>> no longer re-produce >>>> after my previous C++ memory model patch, but this "fix" was more or less >>>> by chance. >>>> >>>> >>>> Boot-Strap on X86_64-pc-linux-gnu (languages=all,ada,go) and >>>> regression-tests >>>> still running. >>>> >>>> >>>> Ok for trunk (when the tests succeed)? >>>> >>>> >>>> Thanks >>>> Bernd.
2013-12-05 Bernd Edlinger <bernd.edlin...@hotmail.de> PR middle-end/59134 * expmed.c (store_bit_field): Use narrow_bit_field_mem and store_fixed_bit_field_1 for -fstrict-volatile-bitfields. (store_fixed_bit_field): Enhanced mode selection algorithm. Call store_fixed_bit_field_1 to do the real work. (store_fixed_bit_field_1): New function. testsuite: 2013-12-05 Bernd Edlinger <bernd.edlin...@hotmail.de> PR middle-end/59134 * gcc.c-torture/compile/pr59134.c: New test. * gnat.dg/misaligned_volatile.adb: New test.
patch-bitfields-update-2.diff
Description: Binary data