| Message ID | 87a9vanaur.fsf@talisman.home |
|---|---|
| State | New |
| Headers | show |
On 10/25/2012 05:45 AM, Richard Sandiford wrote: > Hi Vlad, > > As discussed in the reviews, one of the things that worried me was the > combination of: > > 1) the displacement fixup code in process_address assumes that the address > is exactly equal to BASE_LOC + INDEX_LOC + DISP (with null values > being equivalent to 0). > > 2) extract_address_regs allows (and has to allow) much more general forms > than that. > > 3) equiv_address_substitution folds base and index displacements > without checking the shape of the address. > > So the code from (1) could end up fixing a displacement created by (3) > even if the address isn't a simple BASE_LOC + INDEX_LOC + DISP. > > There's a similar problem with the relationship between INDEX_LOC > and INDEX_REG_LOC: we assume the scale is 1 unless INDEX is a MULT > of the index register. > > This patch adds two utility functions, one to test whether the address > is simple enough for the fixup code to handle, and one to see whether > the index scale is known. The latter handles ASHIFT as well as MULT, > since ASHIFT can be used in out-of-MEM address operands. That is really nice. > The condition: > > /* If the address already has a displacement, we can simply add the > new displacement to it. */ > if (ad->disp_loc) > return true; > > might leave a bit of a hole, but the asserts you added to > extract_address_regs should mean that disp_loc really is a > displacement (thanks for those btw). I am not sure about it. UNSPEC is also treated as a displacement. I don't think it can be combined. > In principle, we could apply a base or index displacement even in cases > that process_address can't fix up, test whether the result is valid, > and revert to the normal behaviour of reloading the full base or index > value if not. That's not going to be useful for x86 (or for MIPS :-)) > and I'm not planning to do it, but the valid_address_p patch I just sent > ought to help. > > Tested on x86_64-linux-gnu. Also tested by making sure that there > were no changes in assembly output for a set of gcc .ii files > (i.e. these extra checks didn't affect the code on x86_64 at least). > OK to install? Probably we should check UNSPEC too. Otherwise, it looks ok. Thanks, Richard. I see a potential bug here. We should not reject new equiv values for base and index here. After we decided to use equiv it should be changed everywhere as we remove init insns. So if we have base + index and each of these is changed by pair of pseudos (I think it is an extremely rare situation) we now have 4 pseudos which we should use. Another situation is pseudo + unspec and we change pseudo by reg+offset. We should deal with this somehow. I guess, we should generate reloads in equiv_address_substitution for such rare cases. > Richard > > > gcc/ > * lra-constraints.c (get_index_scale, can_add_disp_p): New functions. > (equiv_address_substitution): Use them. > > Index: gcc/lra-constraints.c > =================================================================== > --- gcc/lra-constraints.c 2012-10-25 09:50:13.005284668 +0100 > +++ gcc/lra-constraints.c 2012-10-25 09:55:17.376283922 +0100 > @@ -756,6 +756,28 @@ extract_address_regs (enum machine_mode > ad->index_loc = ad->index_reg_loc; > } > > +/* Return the scale applied to *AD->INDEX_REG_LOC, or 0 if the index is > + more complicated than that. */ > +static HOST_WIDE_INT > +get_index_scale (struct address *ad) > +{ > + rtx index = *ad->index_loc; > + if (GET_CODE (index) == MULT > + && CONST_INT_P (XEXP (index, 1)) > + && ad->index_reg_loc == &XEXP (index, 0)) > + return INTVAL (XEXP (index, 1)); > + > + if (GET_CODE (index) == ASHIFT > + && CONST_INT_P (XEXP (index, 1)) > + && ad->index_reg_loc == &XEXP (index, 0)) > + return (HOST_WIDE_INT) 1 << INTVAL (XEXP (index, 1)); > + > + if (ad->index_reg_loc == ad->index_loc) > + return 1; > + > + return 0; > +} > + > > > /* The page contains major code to choose the current insn alternative > @@ -2430,6 +2452,40 @@ base_plus_disp_to_reg (enum machine_mode > return new_reg; > } > > +/* Return true if we can add a displacement to address ADDR_LOC, > + which is described by AD, even if that makes the address invalid. > + The fix-up code requires any new address to be the sum of the base, > + index and displacement fields of an AD-like structure. */ > +static bool > +can_add_disp_p (struct address *ad, rtx *addr_loc) > +{ > + /* Automodified addresses have a fixed form. */ > + if (ad->base_modify_p) > + return false; > + > + /* If the address already has a displacement, we can simply add the > + new displacement to it. */ > + if (ad->disp_loc) > + return true; > + > + /* If the address is entirely a base or index, we can try adding > + a constant to it. */ > + if (addr_loc == ad->base_reg_loc || addr_loc == ad->index_loc) > + return true; > + > + /* Likewise if the address is entirely a sum of the base and index. */ > + if (GET_CODE (*addr_loc) == PLUS) > + { > + rtx *op0 = &XEXP (*addr_loc, 0); > + rtx *op1 = &XEXP (*addr_loc, 1); > + if (op0 == ad->base_reg_loc && op1 == ad->index_loc) > + return true; > + if (op1 == ad->base_reg_loc && op0 == ad->index_loc) > + return true; > + } > + return false; > +} > + > /* Make substitution in address AD in space AS with location ADDR_LOC. > Update AD and ADDR_LOC if it is necessary. Return true if a > substitution was made. */ > @@ -2475,7 +2531,8 @@ equiv_address_substitution (struct addre > } > else if (GET_CODE (new_base_reg) == PLUS > && REG_P (XEXP (new_base_reg, 0)) > - && CONST_INT_P (XEXP (new_base_reg, 1))) > + && CONST_INT_P (XEXP (new_base_reg, 1)) > + && can_add_disp_p (ad, addr_loc)) > { > disp += INTVAL (XEXP (new_base_reg, 1)); > *ad->base_reg_loc = XEXP (new_base_reg, 0); > @@ -2484,12 +2541,6 @@ equiv_address_substitution (struct addre > if (ad->base_reg_loc2 != NULL) > *ad->base_reg_loc2 = *ad->base_reg_loc; > } > - scale = 1; > - if (ad->index_loc != NULL && GET_CODE (*ad->index_loc) == MULT) > - { > - lra_assert (CONST_INT_P (XEXP (*ad->index_loc, 1))); > - scale = INTVAL (XEXP (*ad->index_loc, 1)); > - } > if (index_reg != new_index_reg) > { > if (REG_P (new_index_reg)) > @@ -2499,7 +2550,9 @@ equiv_address_substitution (struct addre > } > else if (GET_CODE (new_index_reg) == PLUS > && REG_P (XEXP (new_index_reg, 0)) > - && CONST_INT_P (XEXP (new_index_reg, 1))) > + && CONST_INT_P (XEXP (new_index_reg, 1)) > + && can_add_disp_p (ad, addr_loc) > + && (scale = get_index_scale (ad))) > { > disp += INTVAL (XEXP (new_index_reg, 1)) * scale; > *ad->index_reg_loc = XEXP (new_index_reg, 0);
On 10/25/2012 04:06 PM, Richard Sandiford wrote: > Vladimir Makarov <vmakarov@redhat.com> writes: >> On 10/25/2012 05:45 AM, Richard Sandiford wrote: >> >> I see a potential bug here. We should not reject new equiv values for >> base and index here. After we decided to use equiv it should be changed >> everywhere as we remove init insns. > Hmm, I might be misunderstanding, sorry, but if we reject them, won't > process_addr_reg sort the equiv thing out instead? That's also what > we do for equiv values that aren't just "reg" or "reg+offset". > > I.e., I thought the displacement handling was just an optimisation > (although a very useful one :-)). I'm not sure whether that answers... > >> So if we have base + index and each of these is changed by pair of >> pseudos (I think it is an extremely rare situation) we now have 4 >> pseudos which we should use. Another situation is pseudo + unspec and >> we change pseudo by reg+offset. We should deal with this somehow. >> >> I guess, we should generate reloads in equiv_address_substitution for >> such rare cases. > ...this too or not. > Sorry, Richard. Please, ignore what I wrote about the potential bug. I already forgot what I did. You are right it is fixed in process_addr_reg.
Vladimir Makarov <vmakarov@redhat.com> writes: > On 10/25/2012 05:45 AM, Richard Sandiford wrote: >> Hi Vlad, >> >> As discussed in the reviews, one of the things that worried me was the >> combination of: >> >> 1) the displacement fixup code in process_address assumes that the address >> is exactly equal to BASE_LOC + INDEX_LOC + DISP (with null values >> being equivalent to 0). >> >> 2) extract_address_regs allows (and has to allow) much more general forms >> than that. >> >> 3) equiv_address_substitution folds base and index displacements >> without checking the shape of the address. >> >> So the code from (1) could end up fixing a displacement created by (3) >> even if the address isn't a simple BASE_LOC + INDEX_LOC + DISP. >> >> There's a similar problem with the relationship between INDEX_LOC >> and INDEX_REG_LOC: we assume the scale is 1 unless INDEX is a MULT >> of the index register. >> >> This patch adds two utility functions, one to test whether the address >> is simple enough for the fixup code to handle, and one to see whether >> the index scale is known. The latter handles ASHIFT as well as MULT, >> since ASHIFT can be used in out-of-MEM address operands. > That is really nice. >> The condition: >> >> /* If the address already has a displacement, we can simply add the >> new displacement to it. */ >> if (ad->disp_loc) >> return true; >> >> might leave a bit of a hole, but the asserts you added to >> extract_address_regs should mean that disp_loc really is a >> displacement (thanks for those btw). > I am not sure about it. UNSPEC is also treated as a displacement. I > don't think it can be combined. OK. As it happens, the patch I just sent (mid-air collision, sorry) deals with those UNSPECs in a different way, so maybe I should hold off and see where that discussion goes. >> In principle, we could apply a base or index displacement even in cases >> that process_address can't fix up, test whether the result is valid, >> and revert to the normal behaviour of reloading the full base or index >> value if not. That's not going to be useful for x86 (or for MIPS :-)) >> and I'm not planning to do it, but the valid_address_p patch I just sent >> ought to help. >> >> Tested on x86_64-linux-gnu. Also tested by making sure that there >> were no changes in assembly output for a set of gcc .ii files >> (i.e. these extra checks didn't affect the code on x86_64 at least). >> OK to install? > Probably we should check UNSPEC too. Otherwise, it looks ok. Thanks, > Richard. > > I see a potential bug here. We should not reject new equiv values for > base and index here. After we decided to use equiv it should be changed > everywhere as we remove init insns. Hmm, I might be misunderstanding, sorry, but if we reject them, won't process_addr_reg sort the equiv thing out instead? That's also what we do for equiv values that aren't just "reg" or "reg+offset". I.e., I thought the displacement handling was just an optimisation (although a very useful one :-)). I'm not sure whether that answers... > So if we have base + index and each of these is changed by pair of > pseudos (I think it is an extremely rare situation) we now have 4 > pseudos which we should use. Another situation is pseudo + unspec and > we change pseudo by reg+offset. We should deal with this somehow. > > I guess, we should generate reloads in equiv_address_substitution for > such rare cases. ...this too or not. Richard
Richard Sandiford <rdsandiford@googlemail.com> writes: > Vladimir Makarov <vmakarov@redhat.com> writes: >> On 10/25/2012 05:45 AM, Richard Sandiford wrote: >>> Hi Vlad, >>> >>> As discussed in the reviews, one of the things that worried me was the >>> combination of: >>> >>> 1) the displacement fixup code in process_address assumes that the address >>> is exactly equal to BASE_LOC + INDEX_LOC + DISP (with null values >>> being equivalent to 0). >>> >>> 2) extract_address_regs allows (and has to allow) much more general forms >>> than that. >>> >>> 3) equiv_address_substitution folds base and index displacements >>> without checking the shape of the address. >>> >>> So the code from (1) could end up fixing a displacement created by (3) >>> even if the address isn't a simple BASE_LOC + INDEX_LOC + DISP. >>> >>> There's a similar problem with the relationship between INDEX_LOC >>> and INDEX_REG_LOC: we assume the scale is 1 unless INDEX is a MULT >>> of the index register. >>> >>> This patch adds two utility functions, one to test whether the address >>> is simple enough for the fixup code to handle, and one to see whether >>> the index scale is known. The latter handles ASHIFT as well as MULT, >>> since ASHIFT can be used in out-of-MEM address operands. >> That is really nice. >>> The condition: >>> >>> /* If the address already has a displacement, we can simply add the >>> new displacement to it. */ >>> if (ad->disp_loc) >>> return true; >>> >>> might leave a bit of a hole, but the asserts you added to >>> extract_address_regs should mean that disp_loc really is a >>> displacement (thanks for those btw). >> I am not sure about it. UNSPEC is also treated as a displacement. I >> don't think it can be combined. > > OK. As it happens, the patch I just sent (mid-air collision, sorry) > deals with those UNSPECs in a different way, so maybe I should hold > off and see where that discussion goes. Sorry to follow up on myself, but I changed my mind. As far as SVN history goes, I think this stage should be committed as a separate patch, so... >>> In principle, we could apply a base or index displacement even in cases >>> that process_address can't fix up, test whether the result is valid, >>> and revert to the normal behaviour of reloading the full base or index >>> value if not. That's not going to be useful for x86 (or for MIPS :-)) >>> and I'm not planning to do it, but the valid_address_p patch I just sent >>> ought to help. >>> >>> Tested on x86_64-linux-gnu. Also tested by making sure that there >>> were no changes in assembly output for a set of gcc .ii files >>> (i.e. these extra checks didn't affect the code on x86_64 at least). >>> OK to install? >> Probably we should check UNSPEC too. Otherwise, it looks ok. Thanks, >> Richard. ...applied with that change after testing on x86_64-linux-gnu, thanks. Richard
Index: gcc/lra-constraints.c =================================================================== --- gcc/lra-constraints.c 2012-10-25 09:50:13.005284668 +0100 +++ gcc/lra-constraints.c 2012-10-25 09:55:17.376283922 +0100 @@ -756,6 +756,28 @@ extract_address_regs (enum machine_mode ad->index_loc = ad->index_reg_loc; } +/* Return the scale applied to *AD->INDEX_REG_LOC, or 0 if the index is + more complicated than that. */ +static HOST_WIDE_INT +get_index_scale (struct address *ad) +{ + rtx index = *ad->index_loc; + if (GET_CODE (index) == MULT + && CONST_INT_P (XEXP (index, 1)) + && ad->index_reg_loc == &XEXP (index, 0)) + return INTVAL (XEXP (index, 1)); + + if (GET_CODE (index) == ASHIFT + && CONST_INT_P (XEXP (index, 1)) + && ad->index_reg_loc == &XEXP (index, 0)) + return (HOST_WIDE_INT) 1 << INTVAL (XEXP (index, 1)); + + if (ad->index_reg_loc == ad->index_loc) + return 1; + + return 0; +} + /* The page contains major code to choose the current insn alternative @@ -2430,6 +2452,40 @@ base_plus_disp_to_reg (enum machine_mode return new_reg; } +/* Return true if we can add a displacement to address ADDR_LOC, + which is described by AD, even if that makes the address invalid. + The fix-up code requires any new address to be the sum of the base, + index and displacement fields of an AD-like structure. */ +static bool +can_add_disp_p (struct address *ad, rtx *addr_loc) +{ + /* Automodified addresses have a fixed form. */ + if (ad->base_modify_p) + return false; + + /* If the address already has a displacement, we can simply add the + new displacement to it. */ + if (ad->disp_loc) + return true; + + /* If the address is entirely a base or index, we can try adding + a constant to it. */ + if (addr_loc == ad->base_reg_loc || addr_loc == ad->index_loc) + return true; + + /* Likewise if the address is entirely a sum of the base and index. */ + if (GET_CODE (*addr_loc) == PLUS) + { + rtx *op0 = &XEXP (*addr_loc, 0); + rtx *op1 = &XEXP (*addr_loc, 1); + if (op0 == ad->base_reg_loc && op1 == ad->index_loc) + return true; + if (op1 == ad->base_reg_loc && op0 == ad->index_loc) + return true; + } + return false; +} + /* Make substitution in address AD in space AS with location ADDR_LOC. Update AD and ADDR_LOC if it is necessary. Return true if a substitution was made. */ @@ -2475,7 +2531,8 @@ equiv_address_substitution (struct addre } else if (GET_CODE (new_base_reg) == PLUS && REG_P (XEXP (new_base_reg, 0)) - && CONST_INT_P (XEXP (new_base_reg, 1))) + && CONST_INT_P (XEXP (new_base_reg, 1)) + && can_add_disp_p (ad, addr_loc)) { disp += INTVAL (XEXP (new_base_reg, 1)); *ad->base_reg_loc = XEXP (new_base_reg, 0); @@ -2484,12 +2541,6 @@ equiv_address_substitution (struct addre if (ad->base_reg_loc2 != NULL) *ad->base_reg_loc2 = *ad->base_reg_loc; } - scale = 1; - if (ad->index_loc != NULL && GET_CODE (*ad->index_loc) == MULT) - { - lra_assert (CONST_INT_P (XEXP (*ad->index_loc, 1))); - scale = INTVAL (XEXP (*ad->index_loc, 1)); - } if (index_reg != new_index_reg) { if (REG_P (new_index_reg)) @@ -2499,7 +2550,9 @@ equiv_address_substitution (struct addre } else if (GET_CODE (new_index_reg) == PLUS && REG_P (XEXP (new_index_reg, 0)) - && CONST_INT_P (XEXP (new_index_reg, 1))) + && CONST_INT_P (XEXP (new_index_reg, 1)) + && can_add_disp_p (ad, addr_loc) + && (scale = get_index_scale (ad))) { disp += INTVAL (XEXP (new_index_reg, 1)) * scale; *ad->index_reg_loc = XEXP (new_index_reg, 0);
Hi Vlad, As discussed in the reviews, one of the things that worried me was the combination of: 1) the displacement fixup code in process_address assumes that the address is exactly equal to BASE_LOC + INDEX_LOC + DISP (with null values being equivalent to 0). 2) extract_address_regs allows (and has to allow) much more general forms than that. 3) equiv_address_substitution folds base and index displacements without checking the shape of the address. So the code from (1) could end up fixing a displacement created by (3) even if the address isn't a simple BASE_LOC + INDEX_LOC + DISP. There's a similar problem with the relationship between INDEX_LOC and INDEX_REG_LOC: we assume the scale is 1 unless INDEX is a MULT of the index register. This patch adds two utility functions, one to test whether the address is simple enough for the fixup code to handle, and one to see whether the index scale is known. The latter handles ASHIFT as well as MULT, since ASHIFT can be used in out-of-MEM address operands. The condition: /* If the address already has a displacement, we can simply add the new displacement to it. */ if (ad->disp_loc) return true; might leave a bit of a hole, but the asserts you added to extract_address_regs should mean that disp_loc really is a displacement (thanks for those btw). In principle, we could apply a base or index displacement even in cases that process_address can't fix up, test whether the result is valid, and revert to the normal behaviour of reloading the full base or index value if not. That's not going to be useful for x86 (or for MIPS :-)) and I'm not planning to do it, but the valid_address_p patch I just sent ought to help. Tested on x86_64-linux-gnu. Also tested by making sure that there were no changes in assembly output for a set of gcc .ii files (i.e. these extra checks didn't affect the code on x86_64 at least). OK to install? Richard gcc/ * lra-constraints.c (get_index_scale, can_add_disp_p): New functions. (equiv_address_substitution): Use them.