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
