| Message ID | 5082C740.3020706@redhat.com |
|---|---|
| State | New |
| Headers | show |
Hi Vlad, Vladimir Makarov <vmakarov@redhat.com> writes: > @@ -543,23 +544,34 @@ extract_loc_address_regs (bool top_p, en > code1 = GET_CODE (arg1); > } > > + if (CONSTANT_P (arg0) > + || code1 == PLUS || code1 == MULT || code1 == ASHIFT) > + { > + tloc = arg1_loc; > + arg1_loc = arg0_loc; > + arg0_loc = tloc; > + arg0 = *arg0_loc; > + code0 = GET_CODE (arg0); > + arg1 = *arg1_loc; > + code1 = GET_CODE (arg1); > + } When does this happen? Is it from the extract_address_regs calls in equiv_address_substitution, or somewhere else? > /* If this machine only allows one register per address, it > must be in the first operand. */ > if (MAX_REGS_PER_ADDRESS == 1 || code == LO_SUM) > { > - extract_loc_address_regs (false, mode, as, arg0_loc, false, code, > - code1, modify_p, ad); > lra_assert (ad->disp_loc == NULL); > ad->disp_loc = arg1_loc; > + extract_loc_address_regs (false, mode, as, arg0_loc, false, code, > + code1, modify_p, ad); > } > /* Base + disp addressing */ > - else if (code != PLUS && code0 != MULT && code0 != ASHIFT > + else if (code0 != PLUS && code0 != MULT && code0 != ASHIFT > && CONSTANT_P (arg1)) > { > - extract_loc_address_regs (false, mode, as, arg0_loc, false, PLUS, > - code1, modify_p, ad); > lra_assert (ad->disp_loc == NULL); > ad->disp_loc = arg1_loc; > + extract_loc_address_regs (false, mode, as, arg0_loc, false, PLUS, > + code1, modify_p, ad); > } > /* If index and base registers are the same on this machine, > just record registers in any non-constant operands. We Wasn't sure: is the order of the disp_loc assignment and extract_loc_address_regs call significant here, or did you just swap them for cosmetic reasons? The original order seemed stronger on the face of it, because we assert that the recursive call hasn't also installed a displacement. > @@ -624,11 +635,18 @@ extract_loc_address_regs (bool top_p, en > > case MULT: > case ASHIFT: > - extract_loc_address_regs (false, mode, as, &XEXP (*loc, 0), true, > - outer_code, code, modify_p, ad); > - lra_assert (ad->index_loc == NULL); > - ad->index_loc = loc; > - break; > + { > + rtx *arg0_loc = &XEXP (x, 0); > + enum rtx_code code0 = GET_CODE (*arg0_loc); > + > + if (code0 == CONST_INT) > + arg0_loc = &XEXP (x, 1); > + extract_loc_address_regs (false, mode, as, arg0_loc, true, > + outer_code, code, modify_p, ad); > + lra_assert (ad->index_loc == NULL); > + ad->index_loc = loc; > + break; > + } Is this just for the MULT case? Treating X in (ashift 1 X) as an index register seems odd on the face of it. > @@ -1757,10 +1786,10 @@ process_alt_operands (int only_alternati > clobber operand if the matching operand is > not dying in the insn. */ > if (! curr_static_id->operand[m].early_clobber > - /*|| operand_reg[nop] == NULL_RTX > + || operand_reg[nop] == NULL_RTX > || (find_regno_note (curr_insn, REG_DEAD, > REGNO (operand_reg[nop])) > - != NULL_RTX)*/) > + != NULL_RTX)) > match_p = true; > } > if (match_p) Ah, so did you find the reason why this was done? :-) > Index: lra.c > =================================================================== > --- lra.c (revision 192634) > +++ lra.c (working copy) > @@ -2035,7 +2035,8 @@ check_rtl (bool final_p) > as legitimate. Although they are legitimate if > they satisfies the constraints and will be checked > by insn constraints which we ignore here. */ > - && GET_RTX_CLASS (GET_CODE (XEXP (op, 0))) == RTX_AUTOINC) > + && GET_CODE (XEXP (op, 0)) != UNSPEC > + && GET_RTX_CLASS (GET_CODE (XEXP (op, 0))) != RTX_AUTOINC) > fatal_insn_not_found (insn); > } > } Which case was this needed for? RTX_AUTOINC codes really did seem like a special case because they have their own constraints ("<" and ">"). But why do we have to assume that all unspec addresses are inherently valid? Richard
On 10/21/2012 05:27 AM, Richard Sandiford wrote: > Hi Vlad, Richard, sorry for long delay with the answer. I was really busy all these days trying to fix a lot of GCC testsuite failures (for some ones it was extremely difficult to find failure reasons) and push LRA into trunk. > Vladimir Makarov <vmakarov@redhat.com> writes: >> @@ -543,23 +544,34 @@ extract_loc_address_regs (bool top_p, en >> code1 = GET_CODE (arg1); >> } >> >> + if (CONSTANT_P (arg0) >> + || code1 == PLUS || code1 == MULT || code1 == ASHIFT) >> + { >> + tloc = arg1_loc; >> + arg1_loc = arg0_loc; >> + arg0_loc = tloc; >> + arg0 = *arg0_loc; >> + code0 = GET_CODE (arg0); >> + arg1 = *arg1_loc; >> + code1 = GET_CODE (arg1); >> + } > When does this happen? Is it from the extract_address_regs calls > in equiv_address_substitution, or somewhere else? I did not analyzed this. Sorry, I had no time for this. I just saw it on a broken test. I think the future work would be a support of canonical expressions through all LRA and machine depended code and also address extract rewriting when we figure out the exact syntax of the addresses. >> /* If this machine only allows one register per address, it >> must be in the first operand. */ >> if (MAX_REGS_PER_ADDRESS == 1 || code == LO_SUM) >> { >> - extract_loc_address_regs (false, mode, as, arg0_loc, false, code, >> - code1, modify_p, ad); >> lra_assert (ad->disp_loc == NULL); >> ad->disp_loc = arg1_loc; >> + extract_loc_address_regs (false, mode, as, arg0_loc, false, code, >> + code1, modify_p, ad); >> } >> /* Base + disp addressing */ >> - else if (code != PLUS && code0 != MULT && code0 != ASHIFT >> + else if (code0 != PLUS && code0 != MULT && code0 != ASHIFT >> && CONSTANT_P (arg1)) >> { >> - extract_loc_address_regs (false, mode, as, arg0_loc, false, PLUS, >> - code1, modify_p, ad); >> lra_assert (ad->disp_loc == NULL); >> ad->disp_loc = arg1_loc; >> + extract_loc_address_regs (false, mode, as, arg0_loc, false, PLUS, >> + code1, modify_p, ad); >> } >> /* If index and base registers are the same on this machine, >> just record registers in any non-constant operands. We > Wasn't sure: is the order of the disp_loc assignment and > extract_loc_address_regs call significant here, or did you just swap > them for cosmetic reasons? The original order seemed stronger on the > face of it, because we assert that the recursive call hasn't also > installed a displacement. It was not a cosmetic change. It was a necessary fix for a broken test. >> @@ -624,11 +635,18 @@ extract_loc_address_regs (bool top_p, en >> >> case MULT: >> case ASHIFT: >> - extract_loc_address_regs (false, mode, as, &XEXP (*loc, 0), true, >> - outer_code, code, modify_p, ad); >> - lra_assert (ad->index_loc == NULL); >> - ad->index_loc = loc; >> - break; >> + { >> + rtx *arg0_loc = &XEXP (x, 0); >> + enum rtx_code code0 = GET_CODE (*arg0_loc); >> + >> + if (code0 == CONST_INT) >> + arg0_loc = &XEXP (x, 1); >> + extract_loc_address_regs (false, mode, as, arg0_loc, true, >> + outer_code, code, modify_p, ad); >> + lra_assert (ad->index_loc == NULL); >> + ad->index_loc = loc; >> + break; >> + } > Is this just for the MULT case? Treating X in (ashift 1 X) as an > index register seems odd on the face of it. Yes, right. It is really safe but it should be fixed for code clearness. >> @@ -1757,10 +1786,10 @@ process_alt_operands (int only_alternati >> clobber operand if the matching operand is >> not dying in the insn. */ >> if (! curr_static_id->operand[m].early_clobber >> - /*|| operand_reg[nop] == NULL_RTX >> + || operand_reg[nop] == NULL_RTX >> || (find_regno_note (curr_insn, REG_DEAD, >> REGNO (operand_reg[nop])) >> - != NULL_RTX)*/) >> + != NULL_RTX)) >> match_p = true; >> } >> if (match_p) > Ah, so did you find the reason why this was done? :-) I already wrote you that I am going to restore the code, as I really remember that I added this for purpose to fix some bug on some target. It is better to have a safer code in the trunk. My intention is remove this code again on the branch to figure out details why I did it. In any case, even if this code is necessary, the code should be rewritten to match when the operand is the same independently on presence of REG_DEAD note. >> Index: lra.c >> =================================================================== >> --- lra.c (revision 192634) >> +++ lra.c (working copy) >> @@ -2035,7 +2035,8 @@ check_rtl (bool final_p) >> as legitimate. Although they are legitimate if >> they satisfies the constraints and will be checked >> by insn constraints which we ignore here. */ >> - && GET_RTX_CLASS (GET_CODE (XEXP (op, 0))) == RTX_AUTOINC) >> + && GET_CODE (XEXP (op, 0)) != UNSPEC >> + && GET_RTX_CLASS (GET_CODE (XEXP (op, 0))) != RTX_AUTOINC) >> fatal_insn_not_found (insn); >> } >> } > Which case was this needed for? RTX_AUTOINC codes really did seem like > a special case because they have their own constraints ("<" and ">"). > But why do we have to assume that all unspec addresses are inherently valid? > That was a necessary fix for GCC testsuite regressions on x86/x86-64. The check failed when final_p == false in order words before LRA actually starts its job.
Index: config/i386/i386.c =================================================================== --- config/i386/i386.c (revision 192634) +++ config/i386/i386.c (working copy) @@ -40819,7 +40819,7 @@ ix86_autovectorize_vector_sizes (void) static reg_class_t ix86_spill_class (reg_class_t rclass, enum machine_mode mode) { - if (TARGET_SSE && TARGET_GENERAL_REGS_SSE_SPILL + if (TARGET_SSE && TARGET_GENERAL_REGS_SSE_SPILL && ! TARGET_MMX && hard_reg_set_subset_p (reg_class_contents[rclass], reg_class_contents[GENERAL_REGS]) && (mode == SImode || (TARGET_64BIT && mode == DImode))) Index: lra-constraints.c =================================================================== --- lra-constraints.c (revision 192634) +++ lra-constraints.c (working copy) @@ -524,6 +524,7 @@ extract_loc_address_regs (bool top_p, en { rtx *arg0_loc = &XEXP (x, 0); rtx *arg1_loc = &XEXP (x, 1); + rtx *tloc; rtx arg0 = *arg0_loc; rtx arg1 = *arg1_loc; enum rtx_code code0 = GET_CODE (arg0); @@ -543,23 +544,34 @@ extract_loc_address_regs (bool top_p, en code1 = GET_CODE (arg1); } + if (CONSTANT_P (arg0) + || code1 == PLUS || code1 == MULT || code1 == ASHIFT) + { + tloc = arg1_loc; + arg1_loc = arg0_loc; + arg0_loc = tloc; + arg0 = *arg0_loc; + code0 = GET_CODE (arg0); + arg1 = *arg1_loc; + code1 = GET_CODE (arg1); + } /* If this machine only allows one register per address, it must be in the first operand. */ if (MAX_REGS_PER_ADDRESS == 1 || code == LO_SUM) { - extract_loc_address_regs (false, mode, as, arg0_loc, false, code, - code1, modify_p, ad); lra_assert (ad->disp_loc == NULL); ad->disp_loc = arg1_loc; + extract_loc_address_regs (false, mode, as, arg0_loc, false, code, + code1, modify_p, ad); } /* Base + disp addressing */ - else if (code != PLUS && code0 != MULT && code0 != ASHIFT + else if (code0 != PLUS && code0 != MULT && code0 != ASHIFT && CONSTANT_P (arg1)) { - extract_loc_address_regs (false, mode, as, arg0_loc, false, PLUS, - code1, modify_p, ad); lra_assert (ad->disp_loc == NULL); ad->disp_loc = arg1_loc; + extract_loc_address_regs (false, mode, as, arg0_loc, false, PLUS, + code1, modify_p, ad); } /* If index and base registers are the same on this machine, just record registers in any non-constant operands. We @@ -575,14 +587,13 @@ extract_loc_address_regs (bool top_p, en extract_loc_address_regs (false, mode, as, arg1_loc, true, PLUS, code0, modify_p, ad); } - /* It might be index * scale + disp. */ - else if (code1 == CONST_INT || code1 == CONST_DOUBLE - || code1 == SYMBOL_REF || code1 == CONST || code1 == LABEL_REF) + /* It might be [base + ]index * scale + disp. */ + else if (CONSTANT_P (arg1)) { lra_assert (ad->disp_loc == NULL); ad->disp_loc = arg1_loc; extract_loc_address_regs (false, mode, as, arg0_loc, context_p, - PLUS, code1, modify_p, ad); + PLUS, code0, modify_p, ad); } /* If both operands are registers but one is already a hard register of index or reg-base class, give the other the @@ -624,11 +635,18 @@ extract_loc_address_regs (bool top_p, en case MULT: case ASHIFT: - extract_loc_address_regs (false, mode, as, &XEXP (*loc, 0), true, - outer_code, code, modify_p, ad); - lra_assert (ad->index_loc == NULL); - ad->index_loc = loc; - break; + { + rtx *arg0_loc = &XEXP (x, 0); + enum rtx_code code0 = GET_CODE (*arg0_loc); + + if (code0 == CONST_INT) + arg0_loc = &XEXP (x, 1); + extract_loc_address_regs (false, mode, as, arg0_loc, true, + outer_code, code, modify_p, ad); + lra_assert (ad->index_loc == NULL); + ad->index_loc = loc; + break; + } case POST_MODIFY: case PRE_MODIFY: @@ -1408,6 +1426,17 @@ simplify_operand_subreg (int nop, enum m alter_subreg (curr_id->operand_loc[nop], false); return true; } + /* Put constant into memory when we have mixed modes. It generates + a better code in most cases as it does not need a secondary + reload memory. It also prevents LRA looping when LRA is using + secondary reload memory again and again. */ + if (CONSTANT_P (reg) && CONST_POOL_OK_P (reg_mode, reg) + && SCALAR_INT_MODE_P (reg_mode) != SCALAR_INT_MODE_P (mode)) + { + SUBREG_REG (operand) = force_const_mem (reg_mode, reg); + alter_subreg (curr_id->operand_loc[nop], false); + return true; + } /* Force a reload of the SUBREG_REG if this is a constant or PLUS or if there may be a problem accessing OPERAND in the outer mode. */ @@ -1415,7 +1444,7 @@ simplify_operand_subreg (int nop, enum m && REGNO (reg) >= FIRST_PSEUDO_REGISTER && (hard_regno = lra_get_regno_hard_regno (REGNO (reg))) >= 0 /* Don't reload paradoxical subregs because we could be looping - having repeatedly final regno out of hard regs range. */ + having repeatedly final regno out of hard regs range. */ && (hard_regno_nregs[hard_regno][GET_MODE (reg)] >= hard_regno_nregs[hard_regno][mode]) && simplify_subreg_regno (hard_regno, GET_MODE (reg), @@ -1757,10 +1786,10 @@ process_alt_operands (int only_alternati clobber operand if the matching operand is not dying in the insn. */ if (! curr_static_id->operand[m].early_clobber - /*|| operand_reg[nop] == NULL_RTX + || operand_reg[nop] == NULL_RTX || (find_regno_note (curr_insn, REG_DEAD, REGNO (operand_reg[nop])) - != NULL_RTX)*/) + != NULL_RTX)) match_p = true; } if (match_p) Index: lra.c =================================================================== --- lra.c (revision 192634) +++ lra.c (working copy) @@ -2035,7 +2035,8 @@ check_rtl (bool final_p) as legitimate. Although they are legitimate if they satisfies the constraints and will be checked by insn constraints which we ignore here. */ - && GET_RTX_CLASS (GET_CODE (XEXP (op, 0))) == RTX_AUTOINC) + && GET_CODE (XEXP (op, 0)) != UNSPEC + && GET_RTX_CLASS (GET_CODE (XEXP (op, 0))) != RTX_AUTOINC) fatal_insn_not_found (insn); } }
After recent patches there were too many regressions of LRA on GCC testsuite on x86 and x86-64. The following patch fixes all of them. It was successfully bootstrapped on x86/x86-64. Committed as rev. 192637. 2012-10-20 Vladimir Makarov <vmakarov@redhat.com> * lra.c (check_rtx): Don't check UNSPEC address. Fix typo with comparing RTX_AUTOINC. * lra-constraints.c (extract_local_address): Swap operands if necessary. Assign to disp before extract_local_address call. Fix typo with PLUS comparison. Use CONSTANT_P. (simplify_operand_subreg): Put constant into memory for subreg with mixed modes. (process_alt_operands): Uncomment code for checking DEAD not for early clobber. * config/i386/i386.c (ix86_spill_class): Don't spill to SSE regs when TARGET_MMX.