Combine changes ASHIFT into mult for non-MEM rtx

[Jeff Law]

On 04/02/2015 03:39 AM, Bin.Cheng wrote:
> Hi,
> In function make_compound_operation, the code/comment says:
>
>      case ASHIFT:
>        /* Convert shifts by constants into multiplications if inside
>       an address.  */
>        if (in_code == MEM && CONST_INT_P (XEXP (x, 1))
>        && INTVAL (XEXP (x, 1)) < HOST_BITS_PER_WIDE_INT
>        && INTVAL (XEXP (x, 1)) >= 0
>        && SCALAR_INT_MODE_P (mode))
>      {
>
>
> Right now, it changes ASHIFT in any SET into mult because of below code:
>
>    /* Select the code to be used in recursive calls.  Once we are inside an
>       address, we stay there.  If we have a comparison, set to COMPARE,
>       but once inside, go back to our default of SET.  */
>
>    next_code = (code == MEM ? MEM
>             : ((code == PLUS || code == MINUS)
>            && SCALAR_INT_MODE_P (mode)) ? MEM         // <--------bogus?
>             : ((code == COMPARE || COMPARISON_P (x))
>            && XEXP (x, 1) == const0_rtx) ? COMPARE
>             : in_code == COMPARE ? SET : in_code);
>
> This seems an overlook to me.  The effect is all targets have to
> support the generated expression in the corresponding pattern.  Some
> times the generated expression is just too stupid and missed.  For
> example below insn is tried by combine:
> (set (reg:SI 79 [ D.2709 ])
>      (plus:SI (subreg:SI (sign_extract:DI (mult:DI (reg:DI 1 x1 [ i ])
>                      (const_int 2 [0x2]))
>                  (const_int 17 [0x11])
>                  (const_int 0 [0])) 0)
>          (reg:SI 0 x0 [ a ])))
>
>
> It actually equals to
> (set (reg/i:SI 0 x0)
>      (plus:SI (ashift:SI (sign_extend:SI (reg:HI 1 x1 [ i ]))
>              (const_int 1 [0x1]))
>          (reg:SI 0 x0 [ a ])))
>
> equals to below instruction on AARCH64:
> add    w0, w0, w1, sxth 1
>
>
> Because of the existing comment, also because it will make backend
> easier (I suppose), is it reasonable to fix this behavior in
> combine.c?  Another question is, if we are going to make the change,
> how many targets might be affected?
There's already a thread on this issue -- it's not a regression so I 
haven't pushed it forward and probably won't until stage1 reopens.

jeff