[PATCH] Strict volatile bit-fields clean-up, Take 2

[Richard Biener]

On Fri, Dec 6, 2013 at 11:15 AM, Bernd Edlinger
<bernd.edlinger@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.edlinger@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 ;)

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.edlinger@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.