| Message ID | B5E67142681B53468FAF6B7C313565623D34C7AA@KLMAIL01.kl.imgtec.org |
|---|---|
| State | New |
| Headers | show |
On 11/20/13 10:27, Robert Suchanek wrote: > Hi, > > I'm pinging for the review of a patch. > > Original post: http://gcc.gnu.org/ml/gcc-patches/2013-11/msg01608.html > > The attached patch is slightly modified as the indentation was incorrect > and realised that after sending the email last time. > > If the patch is OK, could someone commit the patch? I do not have > permission to do so. > > Regards, > Robert > > 2013-11-13 Robert Suchanek <Robert.Suchanek@imgtec.com> > > * lra.c (lra): Set lra_in_progress before check_rtl call. > * recog.c (insn_invalid_p): Add !lra_in_progress to prevent > adding clobber regs when LRA is running Thanks. Vlad may not be available right now, and even if he is, he's probably typing one-handed. So I took care of installing this for you. Thanks, Jeff
>Thanks. Vlad may not be available right now, and even if he is, he's >probably typing one-handed. > >So I took care of installing this for you. > >Thanks, >Jeff Thanks! Regards, Robert
On Wed, Nov 20, 2013 at 11:18:49AM -0700, Jeff Law wrote: > >2013-11-13 Robert Suchanek <Robert.Suchanek@imgtec.com> > > > > * lra.c (lra): Set lra_in_progress before check_rtl call. > > * recog.c (insn_invalid_p): Add !lra_in_progress to prevent > > adding clobber regs when LRA is running Trying to run the testsuite with -mlra and the default -mcmodel=medium on powerpc64 now results in enormous numbers of failures like the following. /home/alanm/src/gcc-virgin/libatomic/testsuite/libatomic.c/atomic-exchange-1.c:67:1: error: insn does not satisfy its constraints: } ^ (insn 5 2 6 2 (set (reg/f:DI 212) (mem/u/c:DI (unspec:DI [ (symbol_ref/u:DI ("*.LC0") [flags 0x2]) (reg:DI 2 2) ] UNSPEC_TOCREL) [0 S8 A8])) /home/alanm/src/gcc-virgin/libatomic/testsuite/libatomic.c/atomic-exchange-1.c:14 505 {*movdi_internal64} (expr_list:REG_EQUAL (symbol_ref:DI ("v") <var_decl 0x3fff9bd00850 v>) (nil))) This is due to that innocuous seeming change of setting lra_in_progress before calling check_rtl(), in combination with previous changes Vlad made to the rs6000 backend here: http://gcc.gnu.org/ml/gcc-patches/2013-10/msg02208.html In particular the "Call legitimate_constant_pool_address_p in strict mode for LRA" change, that sets "strict" when lra_in_progress. I'm not at all familiar with lra so why Vlad made those changes to rs6000.c is totally opaque to me. If this were a reload problem I could dive in and fix it, but not lra, sorry.. What I can say is that the rtl shown above is a toc reference of the form that is valid for -mcmodel=small both before and after reload, and generates "ld offset(r2)" machine instructions. The form is valid for -mcmodel=medium/large only before reload. After reload it is supposed to be split into high/lo_sum variants that generate "addis rtmp,offset@ha(r2); ld offset@l(rtmp)".
On 11/28/2013, 6:50 PM, Alan Modra wrote: > On Wed, Nov 20, 2013 at 11:18:49AM -0700, Jeff Law wrote: >>> 2013-11-13 Robert Suchanek <Robert.Suchanek@imgtec.com> >>> >>> * lra.c (lra): Set lra_in_progress before check_rtl call. >>> * recog.c (insn_invalid_p): Add !lra_in_progress to prevent >>> adding clobber regs when LRA is running > > Trying to run the testsuite with -mlra and the default -mcmodel=medium > on powerpc64 now results in enormous numbers of failures like the > following. > > /home/alanm/src/gcc-virgin/libatomic/testsuite/libatomic.c/atomic-exchange-1.c:67:1: error: insn does not satisfy its constraints: > } > ^ > (insn 5 2 6 2 (set (reg/f:DI 212) > (mem/u/c:DI (unspec:DI [ > (symbol_ref/u:DI ("*.LC0") [flags 0x2]) > (reg:DI 2 2) > ] UNSPEC_TOCREL) [0 S8 A8])) /home/alanm/src/gcc-virgin/libatomic/testsuite/libatomic.c/atomic-exchange-1.c:14 505 {*movdi_internal64} > (expr_list:REG_EQUAL (symbol_ref:DI ("v") <var_decl 0x3fff9bd00850 v>) > (nil))) > > This is due to that innocuous seeming change of setting > lra_in_progress before calling check_rtl(), in combination with > previous changes Vlad made to the rs6000 backend here: > http://gcc.gnu.org/ml/gcc-patches/2013-10/msg02208.html > In particular the "Call legitimate_constant_pool_address_p in strict > mode for LRA" change, that sets "strict" when lra_in_progress. > > I'm not at all familiar with lra so why Vlad made those changes to > rs6000.c is totally opaque to me. If this were a reload problem I > could dive in and fix it, but not lra, sorry.. > > What I can say is that the rtl shown above is a toc reference of the > form that is valid for -mcmodel=small both before and after reload, > and generates "ld offset(r2)" machine instructions. The form is valid > for -mcmodel=medium/large only before reload. After reload it is > supposed to be split into high/lo_sum variants that generate > "addis rtmp,offset@ha(r2); ld offset@l(rtmp)". > Thanks, Alan. I suspected that might be a problem with the patch but I did not expected so many problems (s390 is completely broken too). I've just stated to work on this. I hope it will be fixed today.
On 11/28/13 16:50, Alan Modra wrote: > > This is due to that innocuous seeming change of setting > lra_in_progress before calling check_rtl(), in combination with > previous changes Vlad made to the rs6000 backend here: > http://gcc.gnu.org/ml/gcc-patches/2013-10/msg02208.html > In particular the "Call legitimate_constant_pool_address_p in strict > mode for LRA" change, that sets "strict" when lra_in_progress. Is this still an issue? That code has gone through a couple revisions. Robert's change was reverted and Vlad twiddled thigns to use recog_memoized instead of insn_invalid_p which prevents check_rtl from incorrectly adding CLOBBERs after the point where an insn's form is supposed to be fixed. jeff
On Mon, Dec 02, 2013 at 11:04:39PM -0700, Jeff Law wrote: > On 11/28/13 16:50, Alan Modra wrote: > > > >This is due to that innocuous seeming change of setting > >lra_in_progress before calling check_rtl(), in combination with > >previous changes Vlad made to the rs6000 backend here: > >http://gcc.gnu.org/ml/gcc-patches/2013-10/msg02208.html > >In particular the "Call legitimate_constant_pool_address_p in strict > >mode for LRA" change, that sets "strict" when lra_in_progress. > Is this still an issue? No, the changes Vlad made fixed the powerpc problem. Thanks! > That code has gone through a couple revisions. Robert's change was > reverted and Vlad twiddled thigns to use recog_memoized instead of > insn_invalid_p which prevents check_rtl from incorrectly adding > CLOBBERs after the point where an insn's form is supposed to be > fixed.
diff --git a/gcc/lra.c b/gcc/lra.c index 1aea599..83d45b6 100644 --- a/gcc/lra.c +++ b/gcc/lra.c @@ -2238,6 +2238,10 @@ lra (FILE *f) init_insn_recog_data (); + /* We can not set up reload_in_progress because it prevents new + pseudo creation. */ + lra_in_progress = 1; + #ifdef ENABLE_CHECKING check_rtl (false); #endif @@ -2248,10 +2252,6 @@ lra (FILE *f) setup_reg_spill_flag (); - /* We can not set up reload_in_progress because it prevents new - pseudo creation. */ - lra_in_progress = 1; - /* Function remove_scratches can creates new pseudos for clobbers -- so set up lra_constraint_new_regno_start before its call to permit changing reg classes for pseudos created by this diff --git a/gcc/recog.c b/gcc/recog.c index c8594bb..5c0ec16 100644 --- a/gcc/recog.c +++ b/gcc/recog.c @@ -314,7 +314,9 @@ insn_invalid_p (rtx insn, bool in_group) clobbers. */ int icode = recog (pat, insn, (GET_CODE (pat) == SET - && ! reload_completed && ! reload_in_progress) + && ! reload_completed + && ! reload_in_progress + && ! lra_in_progress) ? &num_clobbers : 0); int is_asm = icode < 0 && asm_noperands (PATTERN (insn)) >= 0;
Hi, I'm pinging for the review of a patch. Original post: http://gcc.gnu.org/ml/gcc-patches/2013-11/msg01608.html The attached patch is slightly modified as the indentation was incorrect and realised that after sending the email last time. If the patch is OK, could someone commit the patch? I do not have permission to do so. Regards, Robert 2013-11-13 Robert Suchanek <Robert.Suchanek@imgtec.com> * lra.c (lra): Set lra_in_progress before check_rtl call. * recog.c (insn_invalid_p): Add !lra_in_progress to prevent adding clobber regs when LRA is running