Re: Validity of SUBREG+AND-imm transformations

[Kyrill Tkachov <kyrylo dot tkachov at foss dot arm dot com>]

Hi Jeff,

On 26/02/16 21:24, Jeff Law wrote:
> On 02/26/2016 06:40 AM, Kyrill Tkachov wrote:
> > Hi all,
> >
> > I'm looking at a case where some RTL passes create an RTL expression of
> > the form:
> > (subreg:QI (and:SI (reg:SI x1)
> >              (const_int 31)) 0)
> >
> > which I'd like to simplify to:
> > (and:QI (subreg:QI (reg:SI x1) 0)
> >          (const_int 31))
> I can think of cases where the first is better and other cases where the second is better -- a lot depends on context.  I don't have a good sense for which is better in general.
>
> Note that as-written these don't trigger the subtle issues in what happens with upper bits.  That's more for extensions.
>
> (subreg:SI (whatever:QI))
>
> vs
>
> {zero,sign}_extend:SI (whatever:QI))
>
> vs
>
> (and:SI (subreg:SI (whatever:QI) (const_int 0x255)))
>
>
> The first leave the bits beyond QI as "undefined" and sometimes (but I doubt all that often in practice) the compiler will use the undefined nature of those bits to enable optimizations.
>
>
> The second & 3rd variants crisply define the upper bits.

Thanks for the explanation.

> > It's easy enough to express in RTL but I'm trying to convince myself on
> > its validity.
> > I know there are some subtle points in this area. combine_simplify_rtx
> > in combine.c
> > has a comment:
> >        /* Note that we cannot do any narrowing for non-constants since
> >       we might have been counting on using the fact that some bits were
> >       zero.  We now do this in the SET.  */
> That comment makes no sense.  Unfortunately it goes back to a change from Kenner in 1994 -- which predates having patch discussions here and consistently adding tests to the testsuite.
>
> The code used to do this:
>
>
> -      if (GET_MODE_CLASS (mode) == MODE_INT
> -         && GET_MODE_CLASS (GET_MODE (SUBREG_REG (x))) == MODE_INT
> -         && GET_MODE_SIZE (mode) < GET_MODE_SIZE (GET_MODE (SUBREG_REG (x)))
> -         && subreg_lowpart_p (x))
> -       return force_to_mode (SUBREG_REG (x), mode, GET_MODE_MASK (mode),
> -                             NULL_RTX, 0);
>
> Which appears to check that we've got a narrowing subreg expression, and if we do try to force the SUBREG_REG into the right mode using force_to_mode.
>
> But if we had a narrowing SUBREG_REG, then I can't see how anything would have been dependign on the upper bits being zero.
>
>
> > and if I try to implement this transformation in simplify_subreg from
> > simplify-rtx.c
> > I get some cases where combine goes into an infinite recursion in
> > simplify_comparison
> > because it tries to do:
> >
> >        /* If this is (and:M1 (subreg:M1 X:M2 0) (const_int C1)) where C1
> >           fits in both M1 and M2 and the SUBREG is either paradoxical
> >           or represents the low part, permute the SUBREG and the AND
> >           and try again.  */
> Right.  I think you just end up ping-ponging between the two equivalent representations.  Which may indeed argue that the existing representation is preferred and we should look deeper into why the existing representation isn't being 
> handled as well as it should be.

After a bit more experimentation I found it's easy enough to avoid looping on this spot.
Just check if the gen_lowpart part of that transformation returned something different from the original
expression...

> > Performing this transformation would help a lot with recognition of some
> > patterns that
> > I'm working on, so would it be acceptable to teach combine or
> > simplify-rtx to do this?
> How does it help recognition?   What kinds of patterns are you trying to recognize?

On aarch64 the normal integer-side variable shift/rotate instructions truncate their shift amount.
However, we can't enable SHIFT_COUNT_TRUNCATED by default on aarch64 because some of the alternatives
in our shift patterns use the vector shift instructions, which don't truncate the shift amount.
So I'm trying to add a combine pattern to eliminate redundant truncations where possible when using
the normal shift instructions.

So I'm trying to create a define_insn to match something like:
  [(set (match_operand:SI 0 "register_operand" "=r")
    (ashift:SI
      (match_operand:SI 1 "register_operand" "r")
      (and:QI
        (match_operand:QI 2 "register_operand" "r")
        (match_operand:QI 3 "const_int_operand" "n"))))]


where operands[3] is 31 for SImode. The 'and' expression has to be in QImode because our shift expanders
expand the shift amount to QImode.
However, when combine tries to combine the and-mask operation with the shift, for example from the code:
unsigned f1(unsigned x, int y) { return x << (y & 31); }

the expression it tries to match is:
(set (reg/i:SI 0 x0)
    (ashift:SI (reg:SI 0 x0 [ x ])
        (subreg:QI (and:SI (reg:SI 1 x1 [ y ])
                (const_int 31 [0x1f])) 0)))

That is, the subreg is not propagated inside. I think the define_insn pattern for that would look
somewhat unnatural, so I'm trying to get the subreg to be propagated to the reg inside.

Thanks,
Kyrill