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

[Bernd Edlinger <bernd dot edlinger at hotmail dot de>]

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.

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.

Attachment: changelog-bitfields-update-2.txt
Description: Text document

Attachment: patch-bitfields-update-2.diff
Description: Binary data