Hi,
On Mon, 9 Dec 2013 14:18:23, Richard Biener wrote:
>
> 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.
>
Great!
Then I think we can put all bits together now:
1. Let Sandra apply her Bit-fields patch "reimplement
-fstrict-volatile-bitfields v4, part 1/2" which was
posted here: http://gcc.gnu.org/ml/gcc-patches/2013-09/msg02058.html
and approved here: http://gcc.gnu.org/ml/gcc-patches/2013-10/msg01476.html
2. As follow-Up I'd like to apply this update-patch, which fixes the
recursion in the extract_split_bit_field and fixes the C++ memory
model for -fstrict-volatile-bitfields:
which was posted here: http://gcc.gnu.org/ml/gcc-patches/2013-11/msg02046.html
and approved here: http://gcc.gnu.org/ml/gcc-patches/2013-12/msg00091.html
3. Then this patch itself "Strict volatile bit-fields clean-up, Take 2".
4. And finally the Clean-up patch: "Strict volatile bit-fields clean-up"
which removes the dependencies on
the variable flag_strict_volatile_bitfields
from expand_assignment and expand_expr_real_1. And uses the access mode
of the field
instead of the structure mode.
which was posted here: http://gcc.gnu.org/ml/gcc-patches/2013-11/msg02479.html
and approved here: http://gcc.gnu.org/ml/gcc-patches/2013-12/msg00086.html
Is that OK?
Thanks
Bernd.
