This is the mail archive of the
mailing list for the GCC project.
Re: [PATCH] [ARM] [RFC] Fix longstanding push_minipool_fix ICE (PR49423, lp1296601)
- From: Richard Earnshaw <rearnsha at arm dot com>
- To: Charles Baylis <charles dot baylis at linaro dot org>
- Cc: Ramana Radhakrishnan <Ramana dot Radhakrishnan at arm dot com>, GCC Patches <gcc-patches at gcc dot gnu dot org>, Maxim Kuvyrkov <maxim dot kuvyrkov at linaro dot org>
- Date: Thu, 03 Jul 2014 15:26:27 +0100
- Subject: Re: [PATCH] [ARM] [RFC] Fix longstanding push_minipool_fix ICE (PR49423, lp1296601)
- Authentication-results: sourceware.org; auth=none
- References: <CADnVucA0vOkQ6BdmCafh8pSoB0ZsQJJd=eaE2Bfkq2sSXVbpEQ at mail dot gmail dot com> <CAJA7tRbR+GJVgZOaKuC+MLx_jqM6_csYaP07Qr_PH0V1ge_sqA at mail dot gmail dot com> <CADnVucD8DMFm9NAKKiMzEirxhxoBCrYdCCjp=tkOtd+vvfTk9g at mail dot gmail dot com> <53B16599 dot 9040003 at arm dot com> <CADnVucA8oNrqkLWrtbYC=RMvC8+CA21Ua=Qfdtmk=nP7ahpx0w at mail dot gmail dot com>
On 02/07/14 13:05, Charles Baylis wrote:
> On 30 June 2014 14:26, Richard Earnshaw <email@example.com> wrote:
>> On 30/06/14 13:53, Charles Baylis wrote:
>>> I see two options to fix it - one is to teach the back-end to
>>> successfully generate code for this insn, and the other is to teach
>>> the back-end that such an insn is not valid. My proposed patch does
>>> the former. The latter can presumably be achieved by providing a
>>> different kind of memory constraint which disallows constant pool
>>> references for these insns although I haven't tried this yet.
>> I think we should be doing the latter (not permitting these operations).
>> If we wanted to do the former, we could just add an offset range for
>> the insn.
>> The reason we don't want the former is that the offset ranges are too
>> small and overly constrain literal pool placement.
> The attached patch adds a 'Uh' constraint, which means the same as
> 'm', except that literal pool references are not allowed. Patterns
> which generate ldr[s]b or ldr[s]h have been updated to use it, and the
> pool_range attributes have been removed from those patterns.
> Bootstrapped and make-checked with no regressions on qemu for
> <date> Charles Baylis <firstname.lastname@example.org>
> PR target/49423
> * config/arm/arm-protos.h (arm_legitimate_address_p,
> arm_is_constant_pool_ref): Add prototypes.
> * config/arm/arm.c (arm_legitimate_address_p): Remove static.
> (arm_is_constant_pool_ref) New function.
> * config/arm/arm.md (unaligned_loadhis, arm_zero_extendhisi2_v6,
> arm_zero_extendqisi2_v6): Use Uh constraint for memory operand.
> (arm_extendhisi2, arm_extendhisi2_v6): Use Uh constraint for memory
> operand and remove pool_range and neg_pool_range attributes.
Better to start a new sentence here.
> (arm_extendqihi_insn, arm_extendqisi, arm_extendqisi_v6): Remove
> pool_range and neg_pool_range attributes.
> * config/arm/constraints.md (Uh): New constraint. (Uq): Don't allow
> constant pool references.
"(Uq)" should be on a new line.
> OK for trunk?
I certainly think this is the right approach.
My only worry is whether we also need new predicates that have similar
restrictions (see, for example how Uq is matched with
arm_extendqisi_mem_op). On balance, I think we're OK without that
change, since I don't expect that we'll see MEM (CONST_POOL_ADDR) unless
reload has already decided to re-materialize a constant register.
So OK, but if you're considering back-ports, I suggest you let it bake a
while on trunk first.