On Thu, Nov 14, 2013 at 11:16 AM, Bernd Edlinger <bernd.edlin...@hotmail.de> wrote: > Hi, > > sorry, for the delay. > Sandra seems to be even more busy than me... > > Attached is a combined patch of the original part 1, and the update, > in diff -up format. > > On Mon, 11 Nov 2013 13:10:45, Richard Biener wrote: >> >> On Thu, Oct 31, 2013 at 1:46 AM, Sandra Loosemore >> <san...@codesourcery.com> wrote: >>> On 10/29/2013 02:51 AM, Bernd Edlinger wrote: >>>> >>>> >>>> On Mon, 28 Oct 2013 21:29:24, Sandra Loosemore wrote: >>>>> >>>>> On 10/28/2013 03:20 AM, Bernd Edlinger wrote: >>>>>> >>>>>> I have attached an update to your patch, that should >>>>>> a) fix the recursion problem. >>>>>> b) restrict the -fstrict-volatile-bitfields to not violate the C++ >>>>>> memory model. >>> >>> >>> Here's a new version of the update patch. >>> >>> >>>>> Alternatively, if strict_volatile_bitfield_p returns false but >>>>> flag_strict_volatile_bitfields> 0, then always force to word_mode and >>>>> change the -fstrict-volatile-bitfields documentation to indicate that's >>>>> the fallback if the insertion/extraction cannot be done in the declared >>>>> mode, rather than claiming that it tries to do the same thing as if >>>>> -fstrict-volatile-bitfields were not enabled at all. >>> >>> >>> I decided that this approach was more expedient, after all. >>> >>> I've tested this patch (in conjunction with my already-approved but >>> not-yet-applied patch) on mainline for arm-none-eabi, x86_64-linux-gnu, and >>> mips-linux gnu. I also backported the entire series to GCC 4.8 and tested >>> there on arm-none-eabi and x86_64-linux-gnu. OK to apply? >> >> Hm, I can't seem to find the context for >> >> @@ -923,6 +935,14 @@ >> store_fixed_bit_field (str_rtx, bitsize, bitnum, 0, 0, value); >> return; >> } >> + else if (MEM_P (str_rtx) >> + && MEM_VOLATILE_P (str_rtx) >> + && flag_strict_volatile_bitfields> 0) >> + /* This is a case where -fstrict-volatile-bitfields doesn't apply >> + because we can't do a single access in the declared mode of the field. >> + Since the incoming STR_RTX has already been adjusted to that mode, >> + fall back to word mode for subsequent logic. */ >> + str_rtx = adjust_address (str_rtx, word_mode, 0); >> >> /* Under the C++0x memory model, we must not touch bits outside the >> bit region. Adjust the address to start at the beginning of the >> >> and the other similar hunk. I suppose they apply to earlier patches >> in the series? I suppose the above applies to store_bit_field (diff -p >> really helps!). Why would using word_mode be any good as >> fallback? That is, why is "Since the incoming STR_RTX has already >> been adjusted to that mode" not the thing to fix? >> > > Well, this hunk does not force the access to be in word_mode. > > Instead it allows get_best_mode to choose the access to be in any mode from > QI to word_mode. > > It is there to revert the effect of this weird code in expr.c > (expand_assigment): > > if (volatilep && flag_strict_volatile_bitfields> 0) > to_rtx = adjust_address (to_rtx, mode1, 0); > > Note that this does not even check if the access is on a bit-field !
Then why not remove that ... > The problem with the strict_volatile_bitfields is that it is used already > before the code reaches store_bit_field or extract_bit_field. > > It starts in get_inner_reference, (which is not only used in expand_assignment > and expand_expr_real_1) > > Then this, > > if (volatilep && flag_strict_volatile_bitfields> 0) > op0 = adjust_address (op0, mode1, 0); ... and this ... > and then this, > > /* If the field is volatile, we always want an aligned > access. Do this in following two situations: > 1. the access is not already naturally > aligned, otherwise "normal" (non-bitfield) volatile fields > become non-addressable. > 2. the bitsize is narrower than the access size. Need > to extract bitfields from the access. */ > || (volatilep && flag_strict_volatile_bitfields> 0 > && (bitpos % GET_MODE_ALIGNMENT (mode) != 0 > || (mode1 != BLKmode > && bitsize < GET_MODE_SIZE (mode1) * BITS_PER_UNIT))) ... or this ... > As a result, a read access to an unaligned volatile data member does > not even reach the expand_bit_field if flag_strict_volatile_bitfields <= 0, > and instead goes through convert_move (target, op0, unsignedp). > > I still believe the proposed patch is guaranteed to not change anything if > -fno-strict-volatile-bitfields is used, and even if we can not guarantee > that it creates exactly the same code for cases where the > strict-volatile-bitfields > does not apply, it certainly generates valid code, where we had invalid code, > or ICEs without the patch. > > OK for trunk? Again, most of the patch is ok (and nice), the store_bit_field/extract_bit_field changes point to the above issues which we should rather fix than trying to revert them after the fact. Why is that not possible? That said, + else if (MEM_P (str_rtx) + && MEM_VOLATILE_P (str_rtx) + && flag_strict_volatile_bitfields > 0) + /* This is a case where -fstrict-volatile-bitfields doesn't apply + because we can't do a single access in the declared mode of the field. + Since the incoming STR_RTX has already been adjusted to that mode, + fall back to word mode for subsequent logic. */ + str_rtx = adjust_address (str_rtx, word_mode, 0); we are looking at an access with bitsize / bitregion properties so plainly choosing word_mode here looks wrong to me. Yes, only -fstrict-volatile-bitfields is affected but still ... The patch is ok if you look at the above as followup. Thanks, Richard. > Bernd. > >> Richard. >> >>> -Sandra
