[ARM] Reintroduce minipool ranges for zero-extension insn patterns

On Tue, 18 Jun 2013 17:10:37 +0100
Richard Earnshaw <rearnsha@arm.com> wrote:

> On 18/06/13 16:42, Julian Brown wrote:
> > Hi,
> >
> > The following patch removed pool_range/neg_pool_range attributes
> > from several instructions as a cleanup, which I believe to have been
> > incorrect:
> >
> > http://gcc.gnu.org/ml/gcc-patches/2010-07/msg01036.html
> >
> > On a Mentor-local branch, this caused problems with instructions
> > like:
> >
> > (insn 77 53 87 (set (reg:SI 8 r8 [orig:197 s.4 ] [197])
> >          (zero_extend:SI (mem/u/c:HI (symbol_ref/u:SI ("*.LC0")
> > [flags 0x2]) [7 S2 A16]))) [...] 161 {*arm_zero_extendhisi2_v6}
> > (nil))
> >
> > The reasoning behind the cleanup was that the instructions in
> > question have no immediate constraints -- but the minipool code is
> > used for more than just immediates, e.g. in the above case where a
> > symbol reference ("m") is loaded.
> >
> > I don't have a test case for the problem on mainline at present,
> > but I believe it is still a latent bug. Tested with the default
> > multilibs (ARM & Thumb mode) on arm-none-eabi, with no regressions.
> > (The patch has also been tested with more multilibs on our local
> > branches for a while, and I did ensure previously that it did not
> > adversely affect Bernd's patch linked above.)
> >
> > OK to apply?
> >
> 
> Pushing extending loads (particularly sign-extending loads) into the 
> minipools adversely affects freedom of pool placement (since the
> offset ranges are limited).
> 
> And they shouldn't be happening anyway.  I really don't think this is 
> the right solution to the problem you have.

How about this as a fix instead? IIUC, all the instances of this bug
that have been reported relate to insns following the pattern:

(set (reg) (sign/zero_extend:SI (symbol_ref:QI/HI)))

where the symbol_ref is a match_operand with an "m" constraint. These
kinds of addresses are explicitly permitted by
arm_legitimate_address_outer_p and thumb2_legitimate_address_p by the
last clause in the functions, "if (GET_MODE_CLASS (mode) != MODE_FLOAT
&& ...", etc.

So, I think the obvious thing to do is to forbid sub-SImode-sized
quantities in that clause. Perhaps tellingly, the thumb1 version of the
function already forbids sizes other than 4 for the mode size in the
equivalent clause. For non-Thumb1, we probably want to retain DImode
(etc.) minipool entries.

The change to generated code for a re-reduced version of the testcase
from:

https://bugzilla.redhat.com/show_bug.cgi?id=927565

(attached), for an insn which breaks before the attached patch is
applied, 210r.reload:

before:

(insn 7 30 33 2 (set (reg:SI 12 ip [orig:110 D.4194 ] [110])
        (zero_extend:SI (mem/u/c:QI (symbol_ref/u:SI ("*.LC0") [flags 0x2]) [0 S1 A8]))) minipool.c:11 182 {*arm_zero_extendqisi2_v6}
     (nil))

after (with a reload):

(insn 221 30 7 2 (set (reg:SI 0 r0)
        (symbol_ref/u:SI ("*.LC0") [flags 0x2])) minipool.c:11 197 {*arm_movsi_insn}
     (nil))
(insn 7 221 33 2 (set (reg:SI 12 ip [orig:110 D.4194 ] [110])
        (zero_extend:SI (mem/u/c:QI (reg:SI 0 r0) [0 S1 A8]))) minipool.c:11 182 {*arm_zero_extendqisi2_v6}
     (nil))

(Compiled with -march=armv7-a -O2.)

Tested cross to arm-none-eabi, default & mthumb multilibs. I also
removed some additional pool_range/neg_pool_ranges from similar
extension instructions, for consistency.

OK to apply?

Thanks,

Julian

ChangeLog

    gcc/
    * config/arm/arm.c (arm_legitimate_address_outer_p)
    (thumb2_legitimate_address_p): Don't allow symbol refs with mode
    size smaller than a word.
    * config/arm/arm.md (thumb1_zero_extendqisi2, *arm_extendhisi2)
    (*arm_extendhisi2_v6, *arm_extendqihi_insn, *arm_extendqisi)
    (*arm_extendqisi_v6): Remove pool_range/neg_pool_range attributes.