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

Richard Biener richard.guenther@gmail.com
Mon Dec 9 13:18:00 GMT 2013


On Mon, Dec 9, 2013 at 1:39 PM, Bernd Edlinger
<bernd.edlinger@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.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 ;)
>>
>
> 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.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.



More information about the Gcc-patches mailing list