[PATCH 2/2] combine: Don't turn (mult (extend x) 2^n) into extract

[Segher Boessenkool]

Hi!

On Mon, Oct 26, 2020 at 10:09:41AM +0000, Alex Coplan wrote:
> On 22/10/2020 15:39, Segher Boessenkool wrote:
> > On Thu, Oct 15, 2020 at 09:59:24AM +0100, Alex Coplan wrote:
> > > Currently, make_extraction() identifies where we can emit an ASHIFT of
> > > an extend in place of an extraction, but fails to make the corresponding
> > > canonicalization/simplification when presented with a MULT by a power of
> > > two. Such a representation is canonical when representing a left-shifted
> > > address inside a MEM.
> > > 
> > > This patch remedies this situation: after the patch, make_extraction()
> > > now also identifies RTXs such as:
> > > 
> > > (mult:DI (subreg:DI (reg:SI r)) (const_int 2^n))
> > > 
> > > and rewrites this as:
> > > 
> > > (mult:DI (sign_extend:DI (reg:SI r)) (const_int 2^n))
> > > 
> > > instead of using a sign_extract.
> > 
> > That is only correct if SUBREG_PROMOTED_VAR_P is true and
> > SUBREG_PROMOTED_UNSIGNED_P is false for r.  Is that guaranteed to be
> > true here (and how then?)
> 
> Sorry, I didn't give enough context here. For this subreg,
> SUBREG_PROMOTED_VAR_P is not set, so I agree that this transformation in
> isolation is not valid.
> 
> The crucial piece of missing information is that we only make this
> transformation in calls to make_extraction where len = 32 + n and
> pos_rtx = pos = 0 (so we're extracting the bottom 32 + n bits), and
> unsignedp is false (so we're doing a sign_extract).

The high half of a DI subreg of a SI reg is *undefined* if
SUBREG_PROMOTED_VAR_P is not set.  So the code you get as input:

> (ashift:DI (subreg:DI (reg/v:SI 93 [ k ]) 0)
>     (const_int 2 [0x2]))

... is already incorrect.  Please fix that?

> where it is clear that the bottom 34 bits are valid (and the
> higher-order bits are undefined). We then hit the block:

No, only the bottom 32 bits are valid.

> diff --git a/gcc/combine.c b/gcc/combine.c
> index c88382efbd3..fe8eff2b464 100644
> --- a/gcc/combine.c
> +++ b/gcc/combine.c
> @@ -7419,8 +7419,8 @@ expand_compound_operation (rtx x)
>      }
>  
>    /* If we reach here, we want to return a pair of shifts.  The inner
> -     shift is a left shift of BITSIZE - POS - LEN bits.  The outer
> -     shift is a right shift of BITSIZE - LEN bits.  It is arithmetic or
> +     shift is a left shift of MODEWIDTH - POS - LEN bits.  The outer
> +     shift is a right shift of MODEWIDTH - LEN bits.  It is arithmetic or
>       logical depending on the value of UNSIGNEDP.
>  
>       If this was a ZERO_EXTEND or ZERO_EXTRACT, this pair of shifts will be

MODEWIDTH isn't defined here yet, it is initialised just below to
MODE_PRECISION (mode).


Segher