[PATCH] lra: Canonicalize mult to shift in address reloads

[Maciej W. Rozycki]

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