On Wed, 26 Aug 2020, Vladimir Makarov via Gcc-patches wrote:
> On 2020-08-26 5:06 a.m., Richard Sandiford wrote:
> >
> > I don't think we should we restrict this to (plus (mult X Y) Z),
> > since addresses can be more complicated than that. One way to
> > search for all MULTs is:
> >
> > subrtx_iterator::array_type array;
> > FOR_EACH_SUBRTX (iter, array, x, NONCONST)
> > {
> > rtx x = *iter;
> > if (GET_CODE (x) == MULT && CONST_INT_P (XEXP (x, 1)))
> > ...
> > }
> >
> > (Needs rtl-iter.h)
>
> I am agree it would be nice to process a general case. Alex, you can do this
> as a separate patch if you want.
>
> Richard, thank you for looking at this patch too.
[From <https://gcc.gnu.org/pipermail/gcc-patches/2020-August/552586.html>;
also commit 6b3034eaba83.]
Guys, this triggers a backend's functional regression and an ICE in the
test suite with the LRA conversion I'm currently working on for the VAX
backend. Before I go ahead and paper it over in the backend I'd like to
understand why this change was considered correct in the first place.
Contrary to what the change description suggests the ASHIFT form is not
documented to be the canonical form for constant multiplication involving
a power of two for addresses used outside `mem'. What our rules only say
is that for addresses inside `mem' the MULT form is:
* Within address computations (i.e., inside 'mem'), a left shift is
converted into the appropriate multiplication by a power of two.
This change does the reverse of the conversion described above and makes
TARGET_LEGITIMATE_ADDRESS_P and possibly other backend code be presented
with either form for indexed addresses, which complicates handling. The
ICE mentioned above specifically is caused by:
(plus:SI (plus:SI (mult:SI (reg:SI 30 [ _10 ])
(const_int 4 [0x4]))
(reg/f:SI 26 [ _6 ]))
(const_int 12 [0xc]))
coming from:
(insn 58 57 59 10 (set (reg:SI 33 [ _13 ])
(zero_extract:SI (mem:SI (plus:SI (plus:SI (mult:SI (reg:SI 30 [ _10 ])
(const_int 4 [0x4]))
(reg/f:SI 26 [ _6 ]))
(const_int 12 [0xc])) [4 _6->bits[_10]+0 S4 A32])
(reg:QI 56)
(reg:SI 53)))
".../gcc/testsuite/gcc.c-torture/execute/20090113-2.c":64:12 490
{*extzv_non_const}
(expr_list:REG_DEAD (reg:QI 56)
(expr_list:REG_DEAD (reg:SI 53)
(expr_list:REG_DEAD (reg:SI 30 [ _10 ])
(expr_list:REG_DEAD (reg/f:SI 26 [ _6 ])
(nil))))))
being converted into:
(plus:SI (plus:SI (ashift:SI (reg:SI 30 [ _10 ])
(const_int 2 [0x2]))
(reg/f:SI 26 [ _6 ]))
(const_int 12 [0xc]))
which the backend currently does not recognise as a valid machine address
and clearly all the fallback handling fails in this case. It also causes
indexed addressing for non-byte data (scaling is implicit in the VAX ISA)
to cease being used where no ICE actually triggers, which causes a serious
code quality regression from extraneous manual address calculations.
Given that the VAX backend currently does not expect ASHIFT in addresses
and it works, this single piece in LRA must be the only one across the
middle end that uses this form and all the other code must have stuck to
the MULT form. So it does not appear to me that ASHIFT form indeed used
not to be considered canonical until this problematic change.
I have looked into what other backends do that support scaled indexed
addressing and x86 escaped a regression here owing to an unrelated much
earlier fix: <https://gcc.gnu.org/ml/gcc-patches/2010-04/msg01170.html>
for PR target/43766 (commit 90f775a9c7af) that added ASHIFT support to its
TARGET_LEGITIMATE_ADDRESS_P handler, and Aarch64 presumably has always had
it.
I have therefore made an experimental change for the VAX backend to
accept ASHIFT in its TARGET_LEGITIMATE_ADDRESS_P handler and just like
reverting this change it makes the ICE go away and indexed addressing to
be used again. However there are numerous other places across the VAX
backend that expect addresses to be in the MULT from, including in
particular expression cost calculation, and it is not clear to me if they
all have to be adjusted for the possibility created by this change for
addresses to come in either form.
So why do we want to use a different canonical form for addresses
depending on the context they are used with?
It does complicate handling in the backend and my understanding has been
that canonicalisation is meant to simplify handling throughout instead.
And sadly the change description does not explain why it is correct to
have addresses use the ASHIFT form in certain contexts and the MULT form
in the remaining ones.
Maciej
