Message ID | 52A89786.20605@redhat.com |
---|---|
State | New |
Headers | show |
On 12/11/13 09:49, Vladimir Makarov wrote: > > I could prevent use validize_mem in rs6000.c but I prefer to do it > in change_addres_1 as other targets might have the same problem and it > is better to have one for all solution. It's certainly been speculated that a fair amount of the "target dependent" stuff done in LEGITIMIZE_ADDRESS and RELOAD_LEGITIMIZE_ADDRESS could be factored out, parameterized and done in a target independent way. > > Still it does not fully solve the problem as insn > r257:DI=[unspec[`*.LC29',%2:DI] 45] cant be recognized as > *movdi... pattern has operand predicates rejecting memory because of > invalid address. To fix this a change in general_operand is done. As > LRA can not work properly with regular insn recognition, I added an > assert for this in lra_set_insn_recog_data to figure out this > situation earlier. I'm getting a bit concerned about the number of places outside LRA that test lra_in_progress. In theory, other code shouldn't want/need to care about LRA vs reload. Both serve the same purpose, lra just does it better :-) Right now the numbers are pretty comparable, but let's try and avoid sprinkling this stuff too far and wide. I can see how these might be needed from time to time > > Again, LRA has a very good code for legitimize address by itself and > it is better to use it. I have no trouble believing that. LEGITIMIZE_ADDRESS/LEGITIMIZE_RELOAD_ADDRESS were really hacks to avoid generating stupid code without having to do any serious surgery, particularly in reload. > The patch was successfully bootstrapped and tested on i686, x86_64, > and PPC64. > > Ok to commit? > > > 2013-12-11 Vladimir Makarov <vmakarov@redhat.com> > > PR rtl-optimization/59466 > * emit-rtl.c (change_address_1): Don't validate address for LRA. > * recog.c (general_operand): Accept any memory for LRA. > * lra.c (lra_set_insn_recog_data): Add an assert. > > Index: emit-rtl.c > =================================================================== > --- emit-rtl.c (revision 205870) > +++ emit-rtl.c (working copy) > @@ -1951,7 +1951,9 @@ change_address_1 (rtx memref, enum machi > && (!validate || memory_address_addr_space_p (mode, addr, as))) > return memref; > > - if (validate) > + /* Don't validate address for LRA. LRA can make the address valid > + by itself in most efficient way. */ > + if (validate && !lra_in_progress) Presumably if LRA can't find a reasonable way to make the address valid, it falls back to copying the address into a register? OK for the trunk. Thanks, jeff
On 12/16/2013, 11:44 AM, Jeff Law wrote: > On 12/11/13 09:49, Vladimir Makarov wrote: >> >> I could prevent use validize_mem in rs6000.c but I prefer to do it >> in change_addres_1 as other targets might have the same problem and it >> is better to have one for all solution. > It's certainly been speculated that a fair amount of the "target > dependent" stuff done in LEGITIMIZE_ADDRESS and > RELOAD_LEGITIMIZE_ADDRESS could be factored out, parameterized and done > in a target independent way. > > >> >> Still it does not fully solve the problem as insn >> r257:DI=[unspec[`*.LC29',%2:DI] 45] cant be recognized as >> *movdi... pattern has operand predicates rejecting memory because of >> invalid address. To fix this a change in general_operand is done. As >> LRA can not work properly with regular insn recognition, I added an >> assert for this in lra_set_insn_recog_data to figure out this >> situation earlier. > I'm getting a bit concerned about the number of places outside LRA that > test lra_in_progress. In theory, other code shouldn't want/need to care > about LRA vs reload. Both serve the same purpose, lra just does it > better :-) > > Right now the numbers are pretty comparable, but let's try and avoid > sprinkling this stuff too far and wide. > I'd like to keep it under control too. For me it would be uncomfortable to have more lra_in_progress occurrences than reload_in_progress ones. The numbers (with taking this patch into account) are very close but lra_in_progress places are still less than reload_in_progress. > > I can see how these might be needed from time to time > >> >> Again, LRA has a very good code for legitimize address by itself and >> it is better to use it. > I have no trouble believing that. > LEGITIMIZE_ADDRESS/LEGITIMIZE_RELOAD_ADDRESS were really hacks to avoid > generating stupid code without having to do any serious surgery, > particularly in reload. > >> The patch was successfully bootstrapped and tested on i686, x86_64, >> and PPC64. >> >> Ok to commit? >> >> >> 2013-12-11 Vladimir Makarov <vmakarov@redhat.com> >> >> PR rtl-optimization/59466 >> * emit-rtl.c (change_address_1): Don't validate address for LRA. >> * recog.c (general_operand): Accept any memory for LRA. >> * lra.c (lra_set_insn_recog_data): Add an assert. >> >> Index: emit-rtl.c >> =================================================================== >> --- emit-rtl.c (revision 205870) >> +++ emit-rtl.c (working copy) >> @@ -1951,7 +1951,9 @@ change_address_1 (rtx memref, enum machi >> && (!validate || memory_address_addr_space_p (mode, addr, as))) >> return memref; >> >> - if (validate) >> + /* Don't validate address for LRA. LRA can make the address valid >> + by itself in most efficient way. */ >> + if (validate && !lra_in_progress) > Presumably if LRA can't find a reasonable way to make the address valid, > it falls back to copying the address into a register? > > Yes, it can always fall back to putting address into a pseudo if it can not generate a better code. Thanks, Jeff.
Index: emit-rtl.c =================================================================== --- emit-rtl.c (revision 205870) +++ emit-rtl.c (working copy) @@ -1951,7 +1951,9 @@ change_address_1 (rtx memref, enum machi && (!validate || memory_address_addr_space_p (mode, addr, as))) return memref; - if (validate) + /* Don't validate address for LRA. LRA can make the address valid + by itself in most efficient way. */ + if (validate && !lra_in_progress) { if (reload_in_progress || reload_completed) gcc_assert (memory_address_addr_space_p (mode, addr, as)); Index: lra.c =================================================================== --- lra.c (revision 205870) +++ lra.c (working copy) @@ -1072,9 +1072,16 @@ lra_set_insn_recog_data (rtx insn) nop = asm_noperands (PATTERN (insn)); data->operand_loc = data->dup_loc = NULL; if (nop < 0) - /* Its is a special insn like USE or CLOBBER. */ - data->insn_static_data = insn_static_data - = get_static_insn_data (-1, 0, 0, 1); + { + /* Its is a special insn like USE or CLOBBER. We should + recognize any regular insn otherwise LRA can do nothing + with this insn. */ + gcc_assert (GET_CODE (PATTERN (insn)) == USE + || GET_CODE (PATTERN (insn)) == CLOBBER + || GET_CODE (PATTERN (insn)) == ASM_INPUT); + data->insn_static_data = insn_static_data + = get_static_insn_data (-1, 0, 0, 1); + } else { /* expand_asm_operands makes sure there aren't too many Index: recog.c =================================================================== --- recog.c (revision 205870) +++ recog.c (working copy) @@ -1,3 +1,4 @@ + /* Subroutines used by or related to instruction recognition. Copyright (C) 1987-2013 Free Software Foundation, Inc. @@ -1021,8 +1022,12 @@ general_operand (rtx op, enum machine_mo if (! volatile_ok && MEM_VOLATILE_P (op)) return 0; - /* Use the mem's mode, since it will be reloaded thus. */ - if (memory_address_addr_space_p (GET_MODE (op), y, MEM_ADDR_SPACE (op))) + /* Use the mem's mode, since it will be reloaded thus. LRA can + generate move insn with invalid addresses which is made valid + and efficiently calculated by LRA through further numerous + transformations. */ + if (lra_in_progress + || memory_address_addr_space_p (GET_MODE (op), y, MEM_ADDR_SPACE (op))) return 1; }