Re: [Patch,AVR]: Fix PR29560 (map 16-bit shift to 8-bit)

[Georg-Johann Lay <avr at gjlay dot de>]

Richard Henderson wrote:
> On 07/27/2011 10:00 AM, Georg-Johann Lay wrote:
>> Richard Henderson wrote:
>>>> +;; "*ashluqihiqi3.mem"
>>>> +;; "*ashlsqihiqi3.mem"
>>>> +(define_insn_and_split "*ashl<extend_prefix>qihiqi3.mem"
>>>> +  [(set (match_operand:QI 0 "memory_operand" "=m")
>>>> +        (subreg:QI (ashift:HI (any_extend:HI (match_operand:QI 1 "register_operand" "r"))
>>>> +                              (match_operand:QI 2 "register_operand" "r"))
>>>> +                   0))]
>>>> +  "!reload_completed"
>>>> +  { gcc_unreachable(); }
>>> Surely this isn't necessary.  Why would you ever be matching a memory output?
>>>
>>>> +(define_insn_and_split "*ashlhiqi3"
>>>> +  [(set (match_operand:QI 0 "nonimmediate_operand" "=r")
>>>> +        (subreg:QI (ashift:HI (match_operand:HI 1 "register_operand" "0")
>>>> +                              (match_operand:QI 2 "register_operand" "r")) 0))]
>>>> +  "!reload_completed"
>>>> +  { gcc_unreachable(); }
>>> Likewise.
>>>
>>> But the first pattern and the peep2 look good.
>>>
>> It's that what combine comes up with, and combine is not smart enough
>> to find a split point between the mem and the subreg.  I don't know
>> enough of combine, maybe it's because can_create_pseudo_p is false
>> during combine, combine has no spare reg.  A combine-split won't
>> help as it needs a pseudo/spare reg.
> 
> Hmm.  Perhaps.  Have you a test case for this?
> 
> r~

char y;

void shift2 (char x, unsigned char s)
{
    y = x << s;
}


Combiner tries:

...

Trying 9 -> 10:
Failed to match this instruction:
(set (mem/c/i:QI (symbol_ref:HI ("y")  <var_decl 0xb764c180 y>) [0 y+0 S1 A8])
    (subreg:QI (ashift:HI (reg:HI 48 [ x ])
            (reg/v:QI 47 [ s ])) 0))

Trying 7, 9 -> 10:
Failed to match this instruction:
(set (mem/c/i:QI (symbol_ref:HI ("y")  <var_decl 0xb764c180 y>) [0 y+0 S1 A8])
    (subreg:QI (ashift:HI (zero_extend:HI (reg/v:QI 46 [ x ]))
            (reg/v:QI 47 [ s ])) 0))
Successfully matched this instruction:
(set (reg:HI 50)
    (zero_extend:HI (reg/v:QI 46 [ x ])))
Failed to match this instruction:
(set (mem/c/i:QI (symbol_ref:HI ("y")  <var_decl 0xb764c180 y>) [0 y+0 S1 A8])
    (subreg:QI (ashift:HI (reg:HI 50)
            (reg/v:QI 47 [ s ])) 0))
starting the processing of deferred insns
ending the processing of deferred insns

It sees that it can split out the zero_extend but it does not try to factor
out the set, i.e. try to split like this where both instructions would match:

(set (reg:QI foo)
    (subreg:QI (ashift:HI (reg:HI 50)
            (reg/v:QI 47 [ s ])) 0))

(set (mem/c/i:QI (symbol_ref:HI ("y")  <var_decl 0xb764c180 y>) [0 y+0 S1 A8])
     (reg:QI foo))


This is not specific the the test case, I see combine frequently to miss such
opportunities even if just non-volatile memory is involved.

If volatile memory is involved it's even worse because opportunities like
load-modify-store, sign- and zero-extends or extracting/inserting are not
detected, see PR49807.

Johann