| Message ID | VI1PR10MB257324749689D46FB73B5400E4DA0@VI1PR10MB2573.EURPRD10.PROD.OUTLOOK.COM |
|---|---|
| State | New |
| Headers | show |
| Series | [LRA] Fix wrong-code PR 91109 | expand |
On 8/5/19 4:37 PM, Bernd Edlinger wrote: > Hi! > > > PR 91109 is a wrong-code bug, where LRA is using a scratch register > which is actually not available for use, and thus gets clobbered > when it should not. That seems to be mostly because the live range > info of the cloned schatch register is not working the way how update_scrach_ops > sets up the new register. Moreover for the new register there is > a much better alternative free register available, so that just not > trying the re-use the previous hard register assignment solves the problem. > > For more background please see the bugzilla PR 91109. > > Since I am able to reproduce this bug with latest gcc-9 branch, I want > to ask right away, if it is okay to back-port after a while. > > > Boot-strapped and reg-tested on x86_64-pc-linux-gnu and armv7-linux-gnueabihf. > Is it OK for trunk? Thank you for working on the problem which is severe as the wrong code is generated. The patch is ok as an intermediate solution. You can commit it to the trunk and gcc-9 branch. Still I think more work on the PR is needed. If subsequent LRA sub-pass spills some pseudo to assign a hard register to the scratch of the rematerialized insn as it was in the original insn, it might make this rematerialization unprofitable. So I'll think how to avoid the unprofitable rematerialization in such cases and would like to work on this PR more. Please, do not close the PR after committing the patch. I am going to work on it more when stage3 starts.
Hi Jakub, I think this wrong code bug would be good to be fixed in 9.2. Would you like me to go ahead, or should it wait for 9.3 ? Thanks Bernd. On 8/7/19 3:32 PM, Vladimir Makarov wrote: > On 8/5/19 4:37 PM, Bernd Edlinger wrote: >> Hi! >> >> >> PR 91109 is a wrong-code bug, where LRA is using a scratch register >> which is actually not available for use, and thus gets clobbered >> when it should not. That seems to be mostly because the live range >> info of the cloned schatch register is not working the way how update_scrach_ops >> sets up the new register. Moreover for the new register there is >> a much better alternative free register available, so that just not >> trying the re-use the previous hard register assignment solves the problem. >> >> For more background please see the bugzilla PR 91109. >> >> Since I am able to reproduce this bug with latest gcc-9 branch, I want >> to ask right away, if it is okay to back-port after a while. >> >> >> Boot-strapped and reg-tested on x86_64-pc-linux-gnu and armv7-linux-gnueabihf. >> Is it OK for trunk? > > Thank you for working on the problem which is severe as the wrong code is generated. The patch is ok as an intermediate solution. You can commit it to the trunk and gcc-9 branch. > > Still I think more work on the PR is needed. If subsequent LRA sub-pass spills some pseudo to assign a hard register to the scratch of the rematerialized insn as it was in the original insn, it might make this rematerialization unprofitable. So I'll think how to avoid the unprofitable rematerialization in such cases and would like to work on this PR more. > > Please, do not close the PR after committing the patch. I am going to work on it more when stage3 starts. > >
On Fri, Aug 09, 2019 at 10:31:57AM +0000, Bernd Edlinger wrote: > I think this wrong code bug would be good to be fixed in 9.2. > > Would you like me to go ahead, or should it wait for 9.3 ? Wait for 9.2.1 reopening, even if we'd roll another RC, I'd be afraid that for RA changes, especially ones that aren't a severe recent regression like this, there is not enough time to have it tested enough. Jakub
2019-07-30 Bernd Edlinger <bernd.edlinger@hotmail.de> PR tree-optimization/91109 * lra-remat.c (update_scratch_ops): Remove assignment of the hard register. Index: gcc/lra-remat.c =================================================================== --- gcc/lra-remat.c (revision 273767) +++ gcc/lra-remat.c (working copy) @@ -1021,7 +1021,6 @@ get_hard_regs (struct lra_insn_reg *reg, int &nreg static void update_scratch_ops (rtx_insn *remat_insn) { - int hard_regno; lra_insn_recog_data_t id = lra_get_insn_recog_data (remat_insn); struct lra_static_insn_data *static_id = id->insn_static_data; for (int i = 0; i < static_id->n_operands; i++) @@ -1032,17 +1031,9 @@ update_scratch_ops (rtx_insn *remat_insn) int regno = REGNO (*loc); if (! lra_former_scratch_p (regno)) continue; - hard_regno = reg_renumber[regno]; *loc = lra_create_new_reg (GET_MODE (*loc), *loc, lra_get_allocno_class (regno), "scratch pseudo copy"); - if (hard_regno >= 0) - { - reg_renumber[REGNO (*loc)] = hard_regno; - if (lra_dump_file) - fprintf (lra_dump_file, " Assigning the same %d to r%d\n", - REGNO (*loc), hard_regno); - } lra_register_new_scratch_op (remat_insn, i, id->icode); }