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?
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. 2013-12-09 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): Split up. Call store_fixed_bit_field_1
to do the real work.
(store_fixed_bit_field_1): New function.
(store_split_bit_field): Limit the unit size to the memory mode size,
to prevent recursion.
testsuite:
2013-12-09 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
