| Message ID | 878v9mdcdl.fsf@talisman.default |
|---|---|
| State | New |
| Headers | show |
On Tue, Nov 27, 2012 at 8:22 PM, Richard Sandiford <rdsandiford@googlemail.com> wrote: > Ramana Radhakrishnan <ramrad01@arm.com> writes: >>> Tested on x86_64-linux-gnu, powerpc64-linux-gnu and mipsisa64-elf. >>> Also tested by making sure that there were no changes in assembly >>> output for a set of gcc .ii files. On the other hand, the -march=octeon >>> output for a set of mips64-linux-gnu gcc .ii files showed the optimisation >>> kicking in as hoped. >> >> This sequence of patches caused a regression in >> gcc.target/arm/volatile-bitfields-1.c . I haven't reviewed the patch >> stack in great detail yet, but it looks like this sequence of patches >> doesn't take the ARM volatile bitfields into account. (193600 is fine, >> 193606 is not). > > Looks like I was wrong to drop the conditions from patch 5. > Please could you try the attached? Fixes volatile-bitfields-1.c but appears to break volatile-bitfields-4.c :( . Ramana > > Richard > > > gcc/ > * expmed.c (adjust_bit_field_mem_for_reg): Add handling of volatile > bitfields. > > Index: gcc/expmed.c > =================================================================== > --- gcc/expmed.c 2012-11-27 19:09:18.000000000 +0000 > +++ gcc/expmed.c 2012-11-27 19:09:33.314634741 +0000 > @@ -372,6 +372,15 @@ adjust_bit_field_mem_for_reg (enum extra > bitregion_end, MEM_ALIGN (op0), > MEM_VOLATILE_P (op0)); > enum machine_mode best_mode; > + if (MEM_VOLATILE_P (op0) && flag_strict_volatile_bitfields > 0) > + { > + while (iter.next_mode (&best_mode)) > + if (GET_MODE_SIZE (best_mode) == MEM_SIZE (op0)) > + return narrow_bit_field_mem (op0, best_mode, bitsize, bitnum, > + new_bitnum); > + return NULL_RTX; > + } > + > if (iter.next_mode (&best_mode)) > { > /* We can use a memory in BEST_MODE. See whether this is true for
On Tue, Nov 27, 2012 at 11:45 PM, Ramana Radhakrishnan <ramana.gcc@googlemail.com> wrote: > On Tue, Nov 27, 2012 at 8:22 PM, Richard Sandiford > <rdsandiford@googlemail.com> wrote: >> Ramana Radhakrishnan <ramrad01@arm.com> writes: >>>> Tested on x86_64-linux-gnu, powerpc64-linux-gnu and mipsisa64-elf. >>>> Also tested by making sure that there were no changes in assembly >>>> output for a set of gcc .ii files. On the other hand, the -march=octeon >>>> output for a set of mips64-linux-gnu gcc .ii files showed the optimisation >>>> kicking in as hoped. >>> >>> This sequence of patches caused a regression in >>> gcc.target/arm/volatile-bitfields-1.c . I haven't reviewed the patch >>> stack in great detail yet, but it looks like this sequence of patches >>> doesn't take the ARM volatile bitfields into account. (193600 is fine, >>> 193606 is not). >> >> Looks like I was wrong to drop the conditions from patch 5. >> Please could you try the attached? > > Fixes volatile-bitfields-1.c but appears to break volatile-bitfields-4.c :( . Can arm folks please re-implement strict volatile bitfields in terms of DECL_BIT_FIELD_REPRESENTATIVE as I suggested elsewhere? Thus, adjust stor-layout.c to compute a proper representative honoring strict volatile bitfield semantics? Thanks, Richard. > Ramana > >> >> Richard >> >> >> gcc/ >> * expmed.c (adjust_bit_field_mem_for_reg): Add handling of volatile >> bitfields. >> >> Index: gcc/expmed.c >> =================================================================== >> --- gcc/expmed.c 2012-11-27 19:09:18.000000000 +0000 >> +++ gcc/expmed.c 2012-11-27 19:09:33.314634741 +0000 >> @@ -372,6 +372,15 @@ adjust_bit_field_mem_for_reg (enum extra >> bitregion_end, MEM_ALIGN (op0), >> MEM_VOLATILE_P (op0)); >> enum machine_mode best_mode; >> + if (MEM_VOLATILE_P (op0) && flag_strict_volatile_bitfields > 0) >> + { >> + while (iter.next_mode (&best_mode)) >> + if (GET_MODE_SIZE (best_mode) == MEM_SIZE (op0)) >> + return narrow_bit_field_mem (op0, best_mode, bitsize, bitnum, >> + new_bitnum); >> + return NULL_RTX; >> + } >> + >> if (iter.next_mode (&best_mode)) >> { >> /* We can use a memory in BEST_MODE. See whether this is true for
On 11/28/12 10:25, Richard Biener wrote: > On Tue, Nov 27, 2012 at 11:45 PM, Ramana Radhakrishnan > <ramana.gcc@googlemail.com> wrote: >> On Tue, Nov 27, 2012 at 8:22 PM, Richard Sandiford >> <rdsandiford@googlemail.com> wrote: >>> Ramana Radhakrishnan <ramrad01@arm.com> writes: >>>>> Tested on x86_64-linux-gnu, powerpc64-linux-gnu and mipsisa64-elf. >>>>> Also tested by making sure that there were no changes in assembly >>>>> output for a set of gcc .ii files. On the other hand, the -march=octeon >>>>> output for a set of mips64-linux-gnu gcc .ii files showed the optimisation >>>>> kicking in as hoped. >>>> >>>> This sequence of patches caused a regression in >>>> gcc.target/arm/volatile-bitfields-1.c . I haven't reviewed the patch >>>> stack in great detail yet, but it looks like this sequence of patches >>>> doesn't take the ARM volatile bitfields into account. (193600 is fine, >>>> 193606 is not). >>> >>> Looks like I was wrong to drop the conditions from patch 5. >>> Please could you try the attached? >> >> Fixes volatile-bitfields-1.c but appears to break volatile-bitfields-4.c :( . > > Can arm folks please re-implement strict volatile bitfields in terms of > DECL_BIT_FIELD_REPRESENTATIVE as I suggested elsewhere? > Thus, adjust stor-layout.c to compute a proper representative honoring > strict volatile bitfield semantics? This is a regression in a feature that used to work before this patch went in. I've now opened PR55516 to track this and in my book this is a P1 regression in a primary port and will break the USB stack in the Linux kernel from previous experience (PR51442) If this has to be rewritten, are you suggesting that this be done in time for 4.8 ? I've found the reference to your previous posts on the subject but on a quick perusal I don't see it as an easy fix. None of us understand the code as well as you do :) OTOH I would have naively expected such a rewrite to be painful in stage3 even if we worked our way around this area. Thanks, Ramana
On Wed, Nov 28, 2012 at 1:06 PM, Ramana Radhakrishnan <ramrad01@arm.com> wrote: > On 11/28/12 10:25, Richard Biener wrote: >> >> On Tue, Nov 27, 2012 at 11:45 PM, Ramana Radhakrishnan >> <ramana.gcc@googlemail.com> wrote: >>> >>> On Tue, Nov 27, 2012 at 8:22 PM, Richard Sandiford >>> <rdsandiford@googlemail.com> wrote: >>>> >>>> Ramana Radhakrishnan <ramrad01@arm.com> writes: >>>>>> >>>>>> Tested on x86_64-linux-gnu, powerpc64-linux-gnu and mipsisa64-elf. >>>>>> Also tested by making sure that there were no changes in assembly >>>>>> output for a set of gcc .ii files. On the other hand, the >>>>>> -march=octeon >>>>>> output for a set of mips64-linux-gnu gcc .ii files showed the >>>>>> optimisation >>>>>> kicking in as hoped. >>>>> >>>>> >>>>> This sequence of patches caused a regression in >>>>> gcc.target/arm/volatile-bitfields-1.c . I haven't reviewed the patch >>>>> stack in great detail yet, but it looks like this sequence of patches >>>>> doesn't take the ARM volatile bitfields into account. (193600 is fine, >>>>> 193606 is not). >>>> >>>> >>>> Looks like I was wrong to drop the conditions from patch 5. >>>> Please could you try the attached? >>> >>> >>> Fixes volatile-bitfields-1.c but appears to break volatile-bitfields-4.c >>> :( . >> >> >> Can arm folks please re-implement strict volatile bitfields in terms of >> DECL_BIT_FIELD_REPRESENTATIVE as I suggested elsewhere? >> Thus, adjust stor-layout.c to compute a proper representative honoring >> strict volatile bitfield semantics? > > > This is a regression in a feature that used to work before this patch went > in. > > I've now opened PR55516 to track this and in my book this is a P1 regression > in a primary port and will break the USB stack in the Linux kernel from > previous experience (PR51442) > > If this has to be rewritten, are you suggesting that this be done in time > for 4.8 ? I've found the reference to your previous posts on the subject but > on a quick perusal I don't see it as an easy fix. None of us understand the > code as well as you do :) > > OTOH I would have naively expected such a rewrite to be painful in stage3 > even if we worked our way around this area. It should correspond to implementing the semantics and warnings in one single place - finish_bitfield_layout. All the RTL expansion code can be removed then. As I am not familiar with the ARM strict volatile bitfield semantics I am of no help here, but I promise to review the patch if you come up with it. Thanks, Richard. > Thanks, > Ramana > >
Index: gcc/expmed.c =================================================================== --- gcc/expmed.c 2012-11-27 19:09:18.000000000 +0000 +++ gcc/expmed.c 2012-11-27 19:09:33.314634741 +0000 @@ -372,6 +372,15 @@ adjust_bit_field_mem_for_reg (enum extra bitregion_end, MEM_ALIGN (op0), MEM_VOLATILE_P (op0)); enum machine_mode best_mode; + if (MEM_VOLATILE_P (op0) && flag_strict_volatile_bitfields > 0) + { + while (iter.next_mode (&best_mode)) + if (GET_MODE_SIZE (best_mode) == MEM_SIZE (op0)) + return narrow_bit_field_mem (op0, best_mode, bitsize, bitnum, + new_bitnum); + return NULL_RTX; + } + if (iter.next_mode (&best_mode)) { /* We can use a memory in BEST_MODE. See whether this is true for