This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [RS6000] Fix PowerPC ICE due to secondary_reload ignoring reload replacements
- From: David Edelsohn <dje dot gcc at gmail dot com>
- To: Alan Modra <amodra at gmail dot com>, Michael Meissner <meissner at linux dot vnet dot ibm dot com>
- Cc: Ulrich Weigand <uweigand at de dot ibm dot com>, GCC Patches <gcc-patches at gcc dot gnu dot org>
- Date: Wed, 9 Sep 2015 21:15:28 -0400
- Subject: Re: [RS6000] Fix PowerPC ICE due to secondary_reload ignoring reload replacements
- Authentication-results: sourceware.org; auth=none
- References: <CAGWvnyk7ZYYBbj0RNg=YhdtZdRnKqhYEgZBymyENhW+99nro4A at mail dot gmail dot com> <20150908124658 dot 8E0D523EC at oc7340732750 dot ibm dot com> <20150910005755 dot GA4330 at bubble dot grove dot modra dot org>
On Wed, Sep 9, 2015 at 8:57 PM, Alan Modra <amodra@gmail.com> wrote:
> On Tue, Sep 08, 2015 at 02:46:58PM +0200, Ulrich Weigand wrote:
>> David Edelsohn wrote:
>> > On Mon, Sep 7, 2015 at 11:47 PM, Alan Modra <amodra@gmail.com> wrote:
>> > > In https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67378 analysis I show
>> > > the reason for this PR is that insns emitted by secondary reload
>> > > patterns are being generated without taking into account other reloads
>> > > that may have occurred. We run into this problem when an insn has a
>> > > pseudo that doesn't get a hard reg, and the pseudo is used in a way
>> > > that requires a secondary reload. In this case the secondary reload
>> > > is needed due to gcc generating a 64-bit gpr load from memory insn
>> > > with an address offset not a multiple of 4.
>> > >
>> > > PR target/67378
>> > > * config/rs6000/rs6000.c (rs6000_secondary_reload_gpr): Find
>> > > reload replacement for PRE_MODIFY address reg.
>> >
>> > I'm okay with this patch, but I'd like Uli to double-check it when he
>> > has a moment.
>>
>> The patch looks OK to me. We definitely need to check for replacements
>> in secondary reload in such cases.
>
> Thanks for reviewing! I think rs6000_secondary_reload_inner needs the
> same. (https://gcc.gnu.org/ml/gcc-patches/2014-11/msg01114.html
> removed a bunch of find_replacement calls that I'd added previously.)
>
> This patch reinstates some of the calls, a little more elegantly than
> in my original effort. I've also corrected an obvious error with the
> PRE_DEC address offset. Bootstrapped and regression tested
> powerpc64le-linux. OK for mainline and gcc-5?
Alan,
You and Mike need to get on the same page. I don't want ping-ponging
patches where you add a check and Mike knowingly or unknowingly
removes it, then you add it back.
Ideally I want a testcase. Barring that, I want a comment at all of
these points explaining why find_replacement is necessary.
Thanks, David