| Message ID | 20160211094540.GB10888@bubble.grove.modra.org |
|---|---|
| State | New |
| Headers | show |
On 02/11/2016 10:45 AM, Alan Modra wrote: > Due to uses elsewhere in vsx instructions, reload chooses to put > psuedo 185 in fr31, which can't be used as a base register in the > following: What code exactly makes the choice of fr31? I assume this is in reg_renumber, so it's IRA and not reload that's making that decision? > PR target/68973 > * reloads.c (find_reloads_address_1): For pre/post-inc/dec > with an invalid hard reg, reload just the reg not the entire > pre/post-inc/dec address expression. Hmm, you're patching tricky code. I'm not sure yet whether this is right or not. More reload dumps might help if you have them; I'll Cc myself on the PR. My gut feeling is that we want to reload the inner reg before entering this block of code, with a new test for SECONDARY_MEMORY_NEEDED alongside the existing block that already sets reloaded_inner_of_autoinc. Bernd
On Thu, Feb 11, 2016 at 03:29:05PM +0100, Bernd Schmidt wrote: > On 02/11/2016 10:45 AM, Alan Modra wrote: > > >Due to uses elsewhere in vsx instructions, reload chooses to put > >psuedo 185 in fr31, which can't be used as a base register in the > >following: > > What code exactly makes the choice of fr31? I assume this is in > reg_renumber, so it's IRA and not reload that's making that decision? Yes, sorry, I shouldn't have said reload chooses the reg. > > PR target/68973 > > * reloads.c (find_reloads_address_1): For pre/post-inc/dec > > with an invalid hard reg, reload just the reg not the entire > > pre/post-inc/dec address expression. > > Hmm, you're patching tricky code. Thanks for being willing to review! :) > I'm not sure yet whether this is right or > not. More reload dumps might help if you have them; I'll Cc myself on the > PR. I've attached rtl dumps to the PR. > My gut feeling is that we want to reload the inner reg before entering this > block of code, Yes, my first quick hack did just that, then I noticed there was existing code to reload the inner reg.. > with a new test for SECONDARY_MEMORY_NEEDED alongside the > existing block that already sets reloaded_inner_of_autoinc. I don't understand this comment. If we're pushing a reload of the inner reg, then the SECONDARY_MEMORY_NEEDED code in push_reload will fire. Why then should there be any need to do anything special in find_reloads_address_1 regarding secondary memory?
On 02/11/2016 08:27 PM, Alan Modra wrote: > On Thu, Feb 11, 2016 at 03:29:05PM +0100, Bernd Schmidt wrote: >> On 02/11/2016 10:45 AM, Alan Modra wrote: >> >>> Due to uses elsewhere in vsx instructions, reload chooses to put >>> psuedo 185 in fr31, which can't be used as a base register in the >>> following: >> >> What code exactly makes the choice of fr31? I assume this is in >> reg_renumber, so it's IRA and not reload that's making that decision? > > Yes, sorry, I shouldn't have said reload chooses the reg. > >>> PR target/68973 >>> * reloads.c (find_reloads_address_1): For pre/post-inc/dec >>> with an invalid hard reg, reload just the reg not the entire >>> pre/post-inc/dec address expression. >> >> Hmm, you're patching tricky code. > > Thanks for being willing to review! :) It's so amazing having Bernd helping out with this stuff. > >> I'm not sure yet whether this is right or >> not. More reload dumps might help if you have them; I'll Cc myself on the >> PR. > > I've attached rtl dumps to the PR. > >> My gut feeling is that we want to reload the inner reg before entering this >> block of code, > > Yes, my first quick hack did just that, then I noticed there was > existing code to reload the inner reg.. I think both approaches can sensibly work. FWIW, there's other ports where this could be a problem. The PA for example often wants to hold integer values in FP regs (due to the integer multiply instruction running in the FP unit). And the PA has auto-inc addressing. There's probably others with those properties. I suspect the same handling is needed in the PRE_MODIFY/POST_MODIFY case immediately before the auto-inc cases. One could argue that handling ought to be generalized and factor'd out rather than (largely) duplicated for the cases. I'll let Bernd own the final review iteration(s) on this issue. jeff
On 02/12/2016 04:27 AM, Alan Modra wrote: > I don't understand this comment. If we're pushing a reload of the > inner reg, then the SECONDARY_MEMORY_NEEDED code in push_reload will > fire. Why then should there be any need to do anything special in > find_reloads_address_1 regarding secondary memory? The idea was to find out whether we needed to special case things. But, after looking at this area for a while, I think your original patch makes the most sense and the things I was worried about are non-issues. Please commit. Bernd
diff --git a/gcc/reload.c b/gcc/reload.c index 6196e63..06426d9 100644 --- a/gcc/reload.c +++ b/gcc/reload.c @@ -5834,14 +5834,16 @@ find_reloads_address_1 (machine_mode mode, addr_space_t as, ? XEXP (x, 0) : reg_equiv_mem (regno)); enum insn_code icode = optab_handler (add_optab, GET_MODE (x)); - if (insn && NONJUMP_INSN_P (insn) && equiv - && memory_operand (equiv, GET_MODE (equiv)) + if (insn && NONJUMP_INSN_P (insn) #if HAVE_cc0 && ! sets_cc0_p (PATTERN (insn)) #endif - && ! (icode != CODE_FOR_nothing - && insn_operand_matches (icode, 0, equiv) - && insn_operand_matches (icode, 1, equiv)) + && (regno < FIRST_PSEUDO_REGISTER + || (equiv + && memory_operand (equiv, GET_MODE (equiv)) + && ! (icode != CODE_FOR_nothing + && insn_operand_matches (icode, 0, equiv) + && insn_operand_matches (icode, 1, equiv)))) /* Using RELOAD_OTHER means we emit this and the reload we made earlier in the wrong order. */ && !reloaded_inner_of_autoinc)