| Message ID | mptmsle9nlp.fsf@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | Tweak base/index disambiguation in decompose_normal_address [PR116236] | expand |
On 8/15/24 2:50 AM, Richard Sandiford wrote: > The PR points out that, for an address like: > > (plus (zero_extend X) Y) > > decompose_normal_address doesn't establish a strong preference > between treating X as the base or Y as the base. As the comment > in the patch says, zero_extend isn't enough on its own to assume > an index, at least not on POINTERS_EXTEND_UNSIGNED targets. > But in a construct like the one above, X and Y have different modes, > and it seems reasonable to assume that the one with the expected > address mode is the base. > > This matters on targets like m68k that support index extension > and that require different classes for bases and indices. > > Tested on aarch64-linux-gnu & x86_64-linux-gnu. Andreas also confirms > that it fixes the m68k LRA problem. OK to install? > > Richard > > > gcc/ > PR middle-end/116236 > * rtlanal.cc (decompose_normal_address): Try to distinguish > bases and indices based on mode, before resorting to "baseness". OK. Thanks to everyone for chasing this down. No idea where we sit with the conversion of m68k to LRA but this looks like it'd be helpful irrespective of that effort. jeff
On Aug 15 2024, Jeff Law wrote: > On 8/15/24 2:50 AM, Richard Sandiford wrote: >> The PR points out that, for an address like: >> (plus (zero_extend X) Y) >> decompose_normal_address doesn't establish a strong preference >> between treating X as the base or Y as the base. As the comment >> in the patch says, zero_extend isn't enough on its own to assume >> an index, at least not on POINTERS_EXTEND_UNSIGNED targets. >> But in a construct like the one above, X and Y have different modes, >> and it seems reasonable to assume that the one with the expected >> address mode is the base. >> This matters on targets like m68k that support index extension >> and that require different classes for bases and indices. >> Tested on aarch64-linux-gnu & x86_64-linux-gnu. Andreas also confirms >> that it fixes the m68k LRA problem. OK to install? >> Richard >> gcc/ >> PR middle-end/116236 >> * rtlanal.cc (decompose_normal_address): Try to distinguish >> bases and indices based on mode, before resorting to "baseness". > OK. Thanks to everyone for chasing this down. No idea where we sit with > the conversion of m68k to LRA but this looks like it'd be helpful > irrespective of that effort. With PR116236 and PR116374 the situation is looking quite well.
On Thu, Aug 15, 2024 at 4:57 PM Jeff Law <jeffreyalaw@gmail.com> wrote: > > > > On 8/15/24 2:50 AM, Richard Sandiford wrote: > > The PR points out that, for an address like: > > > > (plus (zero_extend X) Y) > > > > decompose_normal_address doesn't establish a strong preference > > between treating X as the base or Y as the base. As the comment > > in the patch says, zero_extend isn't enough on its own to assume > > an index, at least not on POINTERS_EXTEND_UNSIGNED targets. > > But in a construct like the one above, X and Y have different modes, > > and it seems reasonable to assume that the one with the expected > > address mode is the base. > > > > This matters on targets like m68k that support index extension > > and that require different classes for bases and indices. > > > > Tested on aarch64-linux-gnu & x86_64-linux-gnu. Andreas also confirms > > that it fixes the m68k LRA problem. OK to install? > > > > Richard > > > > > > gcc/ > > PR middle-end/116236 > > * rtlanal.cc (decompose_normal_address): Try to distinguish > > bases and indices based on mode, before resorting to "baseness". > OK. Thanks to everyone for chasing this down. No idea where we sit > with the conversion of m68k to LRA but this looks like it'd be helpful > irrespective of that effort. I'll point out that this change merely adjusts heuristics and whether there's an underlying issue in the target or LRA remains to be seen? Richard. > jeff >
Richard Biener <richard.guenther@gmail.com> writes: > On Thu, Aug 15, 2024 at 4:57 PM Jeff Law <jeffreyalaw@gmail.com> wrote: >> >> >> >> On 8/15/24 2:50 AM, Richard Sandiford wrote: >> > The PR points out that, for an address like: >> > >> > (plus (zero_extend X) Y) >> > >> > decompose_normal_address doesn't establish a strong preference >> > between treating X as the base or Y as the base. As the comment >> > in the patch says, zero_extend isn't enough on its own to assume >> > an index, at least not on POINTERS_EXTEND_UNSIGNED targets. >> > But in a construct like the one above, X and Y have different modes, >> > and it seems reasonable to assume that the one with the expected >> > address mode is the base. >> > >> > This matters on targets like m68k that support index extension >> > and that require different classes for bases and indices. >> > >> > Tested on aarch64-linux-gnu & x86_64-linux-gnu. Andreas also confirms >> > that it fixes the m68k LRA problem. OK to install? >> > >> > Richard >> > >> > >> > gcc/ >> > PR middle-end/116236 >> > * rtlanal.cc (decompose_normal_address): Try to distinguish >> > bases and indices based on mode, before resorting to "baseness". >> OK. Thanks to everyone for chasing this down. No idea where we sit >> with the conversion of m68k to LRA but this looks like it'd be helpful >> irrespective of that effort. > > I'll point out that this change merely adjusts heuristics and whether there's > an underlying issue in the target or LRA remains to be seen? The PR has a lot more discussion around this. :) The historical interface is that the target can request different register classes for base registers and index registers, but the target doesn't get to choose what it considers to be a base and what it considers to be an index. This is instead determined by target-independent code (like it is for tree-ssa-address.cc, for example). decompose_normal_address wasn't making the same choice between base and index that reload made (or that tree-ssa-address.c would make). Some inconsistencies like that are ok, if both interpretations are valid. But IMO the old decompose_normal_address behaviour was clearly wrong for this case. So I think the patch is fixing a genuine bug, rather than papering over a bug elsewhere. I agree that in some ways it's not a very satisfactory situation, since there's a fair bit of guesswork and inference going on (and is in reload too). But I don't think we can avoid that without changing the interface. Changing the interface would be great, but it's a daunting amount of work, especially given that we have so many inactive ports in-tree that would each need to be updated individually. Thanks, Richard
diff --git a/gcc/rtlanal.cc b/gcc/rtlanal.cc index 4158a531bdd..71207ee4f41 100644 --- a/gcc/rtlanal.cc +++ b/gcc/rtlanal.cc @@ -6724,20 +6724,36 @@ decompose_normal_address (struct address_info *info) } else if (out == 2) { + auto address_mode = targetm.addr_space.address_mode (info->as); + rtx inner_op0 = *inner_ops[0]; + rtx inner_op1 = *inner_ops[1]; + int base; + /* If one inner operand has the expected mode for a base and the other + doesn't, assume that the other one is the index. This is useful + for addresses such as: + + (plus (zero_extend X) Y) + + zero_extend is not in itself enough to assume an index, since bases + can be zero-extended on POINTERS_EXTEND_UNSIGNED targets. But if + Y has address mode and X doesn't, there should be little doubt that + Y is the base. */ + if (GET_MODE (inner_op0) == address_mode + && GET_MODE (inner_op1) != address_mode) + base = 0; + else if (GET_MODE (inner_op1) == address_mode + && GET_MODE (inner_op0) != address_mode) + base = 1; /* In the event of a tie, assume the base comes first. */ - if (baseness (*inner_ops[0], info->mode, info->as, PLUS, - GET_CODE (*ops[1])) - >= baseness (*inner_ops[1], info->mode, info->as, PLUS, - GET_CODE (*ops[0]))) - { - set_address_base (info, ops[0], inner_ops[0]); - set_address_index (info, ops[1], inner_ops[1]); - } + else if (baseness (inner_op0, info->mode, info->as, PLUS, + GET_CODE (*ops[1])) + >= baseness (inner_op1, info->mode, info->as, PLUS, + GET_CODE (*ops[0]))) + base = 0; else - { - set_address_base (info, ops[1], inner_ops[1]); - set_address_index (info, ops[0], inner_ops[0]); - } + base = 1; + set_address_base (info, ops[base], inner_ops[base]); + set_address_index (info, ops[1 - base], inner_ops[1 - base]); } else gcc_assert (out == 0);