Vladimir Makarov <vmaka...@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
