Hi,
are there any more comments on this?
I would like to apply the patch as is, unless we find a
a way to get to a test case, maybe with a cross-compiler,
where the MODE_ALIGNMENT is different from MODE_BITSIZE.
Currently, I think that does not happen.
Thanks
Bernd.
> Date: Tue, 10 Mar 2015 14:40:52 +0100
>
> Hi Richard and Eric,
>
> On Mon, 9 Mar 2015 15:30:31, Richard Biener wrote:
>>> Reg-tested on x86_64 successfully and ARM is still running.
>
> ARM completed without regressions meanwhile.
>
>>>
>>> Is it OK for trunk?
>>
>> Looks ok to me apart from
>>
>> /* 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 % (STRICT_ALIGNMENT ? modesize : BITS_PER_UNIT)
>> + + bitsize> modesize
>> + || (STRICT_ALIGNMENT && MEM_ALIGN (op0) < modesize))
>> return false;
>>
>> where I'd use GET_MODE_ALIGNMENT (fieldmode) rather than modesize
>> (in both places).
>>
>> Please leave Eric the chance to comment.
>>
>
> Just to clarify a few things here:
>
> I try to make the checks in strict_volatile_bitfield_p
> to be consistent with the strict volatile bit-field code path
> that follows if we return true here.
>
> I would summarize the current implementation of the
> strict volatile bit-field code as follows:
>
> For strict alignment, we access the structure as
> if it were an array of fieldmode. A multiple of modesize
> is added to the base address, and one single read and/or
> write access must be sufficient to do the job. The access
> must be Ok regarding the target's alignment restrictions.
> That does not change, what changed with the previous patch
> is a missed optimization with the EP_insv code pattern.
>
> For non strict alignment, a multiple of modesize is added
> to the base address, but if the range [bitnum:bitnum+bitsize-1]
> spans two fieldmode words, which should only happen if we
> use packed structures, a byte offset is added to the base address.
> The byte offset is chosen as small as possible, to not reach beyond
> the bit field region. That is new. This change is irrelevant for the use
> case
> of accessing a device register, but the generated code is more compact.
>
> Usually we have GET_MODE_ALIGNMENT(fieldmode)==modesize
> for all SCALAR_INT_MODE_P(fieldmode).
>
> The only exceptions are complex numbers, and targets where
> ADJUST_ALIGNMENT is used in the modes.def, right?
>
> The targets that do that are few, and the modes are mostly vector modes.
> So I did not find any target where the MODE_ALIGNMENT would make
> a difference here. Therefore I think it is more or less a matter of taste.
> But please correct me if I am wrong.
>
> If there are cases, where MODE_ALIGNMENT<MODE_BITSIZE,
> changing the second condition MEM_ALIGN(op0)<modesize to
> MEM_ALIGN(op0)<GET_MODE_ALIGNMENT(filedmode) would
> still be consistent, and probably be more correct.
>
> But I think changing the first condition would allow cases where this
> assertion in the patch does no longer hold:
> gcc_assert (bitnum + bitsize <= GET_MODE_BITSIZE (fieldmode));
>
>
>
> Thanks
> Bernd.
>
