On Tue, Dec 10, 2013 at 10:31 AM, Bernd Edlinger
<bernd.edlin...@hotmail.de> wrote:
> 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?
If they were all approved yes.
Thanks,
Richard.
> Thanks
> Bernd.
