On Thu, Mar 5, 2015 at 12:00 PM, Bernd Edlinger <bernd.edlin...@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.edlin...@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. > >
