Re: ARM patch (PR42172): Rework the {sign,zero}-extend patterns

[Richard Earnshaw <rearnsha@arm.com>]

On Wed, 2010-06-09 at 00:34 +0200, Bernd Schmidt wrote:
> PR42172 is a case where we generate terrible code for bitfield
> assignments on Thumb1.  Part of the problem is that we fail to optimize
> 
> 	strb	r3, [r0]
> 	ldrb	r3, [r0]
> 
> (insn 11 (set (mem:QI (reg:SI 133) (subreg:QI (reg:SI 136) 0))
> (insn 12 (set (reg:SI 140) (zero_extend:SI (mem:QI (reg/v/f:SI 133)))))
> 
> cse will of course try to replace the second MEM with the previous value
> in a register, but the ARM backend (if !arch_v6) does not accept
> zero_extend from a reg, only from a mem.  This makes some sense at first
> glance as there isn't a specific instruction, so the expander creates a
> sequence of shifts when given a REG input.
> 
> However, to make CSE do its job, I believe it's best to make the ARM
> extension patterns accept registers.  We should avoid creating such
> patterns when possible, both at expand time (generating the sequence of
> shifts as before), and in the combiner (by setting appropriate
> rtx_costs), since the two-shift sequence can often be optimized further.
> 
> The following patch does that, and it also fixes a few other problems:
> 
> * The expanders are a little ugly:
>   [(set (match_dup 2)
> 	(ashift:SI (match_operand:HI 1 "nonimmediate_operand" "")
> 		   (const_int 16)))
>   This is invalid RTL; it works because we modify operand[1], but I've
>   changed the patterns to avoid this (and to make them a lot smaller,
>   and, IMO, clearer)
> * Slightly broken Thumb1 patterns:
>     if (which_alternative == 0)
>       return \"sxtb\\t%0, %1\";
>     [....]
>     if (which_alternative == 0)
>       return \"ldrsb\\t%0, %1\";
> * Removed massive duplication of Thumb1 patterns for v6 and !v6
> * Removed apparently useless code to handle LABEL_REFs in addresses
>   in Thumb1 extend patterns.
> 
> Here are some examples of the code generation changes:
>         strb    r3, [r4, #4]
>         ldrb    r2, [r8, #4]    @ zero_extendqisi2
> -       ldrb    r3, [r4, #4]    @ zero_extendqisi2
> +       and     r3, r3, #255
> ====
> -       ldrb    r3, [sp, #36]   @ zero_extendqisi2
> -       bic     r3, r3, #176
> -       orr     r3, r3, #64
> -       strb    r3, [sp, #36]
> -       ldrb    r3, [sp, #36]   @ zero_extendqisi2
> -       bic     r3, r3, #10
> -       orr     r3, r3, #5
> +       mov     r3, #69
> ====
> -       strb    r2, [ip, #4]
> -       ldrb    r2, [ip, #4]    @ zero_extendqisi2
>         bic     r2, r2, #2
>         strb    r2, [ip, #4]
> ====
> -       ldrh    r3, [r5, #2]
> -       lsl     r3, r3, #16
> -       asr     r3, r3, #16
> -       add     r2, r3, #1
> +       mov     r0, #2
> +       ldrsh   r3, [r5, r0]
> +       add     r1, r3, #1
> 
> On the whole things looked very good in the testcases I looked at.
> Unfortunately PR42172 isn't completely solved by this, but it gets a lot
> better.
> 
> Regression tested on:
> Target is arm-none-linux-gnueabi
> Host   is i686-pc-linux-gnu
> 
> Schedule of variations:
>     qemu-system-armv7/arch=armv7-a/thumb
>     qemu-system-armv7/thumb
>     qemu-system-armv7
> 

This is ok.

Minor coding style nit, however:  

+  if (reg_overlap_mentioned_p (operands[0], addr))
+    {
+      ...
+    }
+  if (REG_P (addr))


There should be a blank line at the end of each if...[else ...]
sequence.

R.