On Mon, Dec 9, 2013 at 1:39 PM, Bernd Edlinger <bernd.edlin...@hotmail.de> wrote: > On Fri, 6 Dec 2013 11:51:15, Richard Biener wrote: >> >> On Fri, Dec 6, 2013 at 11:15 AM, Bernd Edlinger >> <bernd.edlin...@hotmail.de> wrote: >>> 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. >> >> This was rev. 43864 btw, no testcase but added the comment >> "We don't want a mode bigger than the destination". >> >> So isn't the bug fixed by calling your new store_fixed_bit_field_1 >> from store_split_bit_field? In fact that caller knows the mode >> it wants to do the store with, so why not even have an explicit >> mode argument for store_fixed_bit_field_1 instead of extracting >> it from op0? >> >> Note I just want to help as well and I am not very familiar with >> the details of the implementation here. So I'd rather have >> a patch "obviously correct" to me - which expanding a condition >> by several more checks isn't ;) >> > > Hmm... > > I think if I solve it from the other side, everything looks > much more obvious indeed. > > How about this new version: For the inconsistent values, > MODE = QImode, bitpos=11, bitsize=8, it just generates two > QImode accesses now, instead of a single HI or SImode. > > Boot-strapped and regression-test passed on X86-linux-gnu. > > OK for trunk?
Looks good to me. Thanks, Richard. > > Bernd. > >> Richard. >> >>> 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.