Re: [PATCH] Fix another wrong-code bug with -fstrict-volatile-bitfields

[Richard Biener <richard dot guenther at gmail dot com>]

On Thu, Mar 5, 2015 at 12:00 PM, Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:
> Hi,
>
> On Thu, 5 Mar 2015 10:00:26, Richard Biener wrote:
>>
>> On Wed, Mar 4, 2015 at 3:13 PM, Bernd Edlinger
>> <bernd.edlinger@hotmail.de> wrote:
>>> bounced... again, without html.
>>>
>>>
>>> Hi Richard,
>>>
>>> while working on another bug in the area of -fstrict-volatile-bitfields
>>> I became aware of another example where -fstrict-volatile-bitfields may generate
>>> wrong code. This is reproducible on a !STRICT_ALIGNMENT target like x86_64.
>>>
>>> The problem is that strict_volatile_bitfield_p tries to allow more than necessary
>>> if !STRICT_ALIGNMENT. Everything works OK on ARM for instance.
>>>
>>> If this function returns true, we may later call narrow_bit_field_mem, and
>>> the check in strict_volatile_bitfield_p should mirror the logic there:
>>> narrow_bit_field_mem just uses GET_MODE_BITSIZE (mode) and does not
>>> care about STRICT_ALIGNMENT, and in the end *new_bitnum + bitsize may
>>> reach beyond the end of the region. This causes store_fixed_bit_field_1
>>> to silently fail to generate correct code.
>>
>> Hmm, but the comment sounds like if using GET_MODE_ALIGNMENT is
>> more correct (even for !strict-alignment) - if mode is SImode and mode
>> alignment is 16 (HImode aligned) then we don't need to split the load
>> if bitnum is 16 and bitsize is 32.
>>
>> So maybe narrow_bit_field_mem needs to be fixed as well?
>>
>
> I'd rather not touch that function....
>
> In the whole expmed.c the only place where  GET_MODE_ALIGNMENT
> is used, is in simple_mem_bitfield_p but only if SLOW_UNALIGNED_ACCESS
> returns 1, which is only the case on few targets.
> Do you know any targets, where GET_MODE_ALIGNMENT may be less than
> GET_MODE_BITSIZE?

DImode on i?86, I suppose any mode on targets like AVR.

> Maybe one thing is missing from strict_volatile_bitfield_p, I am not sure.
>
> Maybe it should check that MEM_ALIGN (op0)>= GET_MODE_ALIGNMENT (fieldmode)
> Because the strict volatile bitfields handling will inevitably try to use
> the fieldmode to access the memory.
>
> Or would it be better to say MEM_ALIGN (op0)>= GET_MODE_BITSIZE (fieldmode),
> because it is easier to explain when some one asks, when we guarantee the semantics
> of strict volatile bitfield?

But on non-strict-align targets you can even for 1-byte aligned MEMs
access an SImode field directly.  So the old code looks correct to me
here and the fix needs to be done somewhere else.

> Probably there is already something in the logic in expr.c that prevents these cases,
> because otherwise it would be way to easy to find an example for unaligned accesses
> to unaligned memory on STRICT_ALIGNMENT targets.
>
>
> Ok, what would you think about this variant?
>
> --- expmed.c.jj    2015-01-16 11:20:40.000000000 +0100
> +++ expmed.c    2015-03-05 11:50:09.400444416 +0100
> @@ -472,9 +472,11 @@ strict_volatile_bitfield_p (rtx op0, uns
>      return false;
>
>    /* Check for cases of unaligned fields that must be split.  */
> -  if (bitnum % BITS_PER_UNIT + bitsize> modesize
> -      || (STRICT_ALIGNMENT
> -      && bitnum % GET_MODE_ALIGNMENT (fieldmode) + bitsize> modesize))
> +  if (bitnum % modesize + bitsize> modesize)
> +    return false;
> +
> +  /* Check that the memory is sufficiently aligned.  */
> +  if (MEM_ALIGN (op0) < modesize)

I think that only applies to strict-align targets and then only for
GET_MODE_ALIGNMENT (modesize).  And of course what matters
is the alignment at bitnum - even though op0 may be not sufficiently
aligned it may have known misalignment so that op0 + bitnum is
sufficiently aligned.

Testcases would need to annotate structs with packed/aligned attributes
to get at these cases.

For the testcase included in the patch, what does the patch end up doing?
Not going the strict-volatile bitfield expansion path?  That looks unnecessary
on !strict-alignment targets but resonable on strict-align targets where the
access would need to be splitted.  So, why does it end up being splitted
on !strict-align targets?

Richard.


>      return false;
>
>    /* Check for cases where the C++ memory model applies.  */
>
>
> Trying to use an atomic access to a device register is pointless if the
> memory context is not aligned to the MODE_BITSIZE, that has nothing
> to do with MODE_ALIGNMENT, right?
>
>
> Thanks
> Bernd.
>
>