On 26/10/2020 05:48, Segher Boessenkool wrote: > 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.
Well, only the low 32 bits of the subreg are valid. But because those low 32 bits are shifted left 2 times, the low 34 bits of the ashift are valid: the bottom 2 bits of the ashift are zeros, and the 32 bits above those are from the inner SImode reg (with the upper 62 bits being undefined). > > > 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). Yes, but bitsize isn't defined at all in this function AFAICT. Are comments not permitted to refer to variables defined immediately beneath them? Alex
