| Message ID | 56BD354D.3000803@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 02/11/2016 06:28 PM, Bernd Schmidt wrote: > This seems fairly straightforward: > > (insn 213 455 216 6 (set (reg:SI 266) > (mem/u/c:SI (post_inc:SI (reg/f:SI 267)) [4 S4 A32])) 748 > {*thumb1_movsi_insn} > (expr_list:REG_EQUAL (const_int -1044200508 [0xffffffffc1c2c3c4]) > (expr_list:REG_INC (reg/f:SI 267) > (nil)))) > > We don't notice that the SET_SRC has a side effect, record the insn as > an equivalencing one, and later remove it because we replaced the reg > with the constant everywhere. Thus, the increment doesn't take place. > > Fixed as follows. Bootstrapped and tested on x86_64-linux. Also compared > before/after dumps for the testcase with arm-elf. Ok? > > > Bernd > > equiv-inc.diff > > > PR rtl-optimization/69752 > * ira.c (update_equiv_regs): When looking for more than a single SET, > also take other side effects into account. Also note that the reporter says gcc-4.9 didn't have this problem, so there's a reasonable chance this is a latent regression exposed by codegen changes prior to IRA. OK for the trunk. Jeff
On 12/02/16 07:43, Jeff Law wrote: > On 02/11/2016 06:28 PM, Bernd Schmidt wrote: >> This seems fairly straightforward: >> >> (insn 213 455 216 6 (set (reg:SI 266) >> (mem/u/c:SI (post_inc:SI (reg/f:SI 267)) [4 S4 A32])) 748 >> {*thumb1_movsi_insn} >> (expr_list:REG_EQUAL (const_int -1044200508 [0xffffffffc1c2c3c4]) >> (expr_list:REG_INC (reg/f:SI 267) >> (nil)))) >> >> We don't notice that the SET_SRC has a side effect, record the insn as >> an equivalencing one, and later remove it because we replaced the reg >> with the constant everywhere. Thus, the increment doesn't take place. >> >> Fixed as follows. Bootstrapped and tested on x86_64-linux. Also compared >> before/after dumps for the testcase with arm-elf. Ok? >> >> >> Bernd >> >> equiv-inc.diff >> >> >> PR rtl-optimization/69752 >> * ira.c (update_equiv_regs): When looking for more than a single SET, >> also take other side effects into account. > Also note that the reporter says gcc-4.9 didn't have this problem, so > there's a reasonable chance this is a latent regression exposed by > codegen changes prior to IRA. > > > OK for the trunk. > > Jeff > I tested it for the particular testcase it was failing for cortex-m0 and it fixed it. Ill fire up a regression run next. Cheers, Andre
On 12/02/16 07:43, Jeff Law wrote: > On 02/11/2016 06:28 PM, Bernd Schmidt wrote: >> This seems fairly straightforward: >> >> (insn 213 455 216 6 (set (reg:SI 266) >> (mem/u/c:SI (post_inc:SI (reg/f:SI 267)) [4 S4 A32])) 748 >> {*thumb1_movsi_insn} >> (expr_list:REG_EQUAL (const_int -1044200508 [0xffffffffc1c2c3c4]) >> (expr_list:REG_INC (reg/f:SI 267) >> (nil)))) >> >> We don't notice that the SET_SRC has a side effect, record the insn as >> an equivalencing one, and later remove it because we replaced the reg >> with the constant everywhere. Thus, the increment doesn't take place. >> >> Fixed as follows. Bootstrapped and tested on x86_64-linux. Also compared >> before/after dumps for the testcase with arm-elf. Ok? >> >> >> Bernd >> >> equiv-inc.diff >> >> >> PR rtl-optimization/69752 >> * ira.c (update_equiv_regs): When looking for more than a single >> SET, >> also take other side effects into account. Will it be better that we don't remove the insn if it has side-effect instead of don't record the equiv? This can offer more equiv for later rtl optimization? > Also note that the reporter says gcc-4.9 didn't have this problem, so > there's a reasonable chance this is a latent regression exposed by > codegen changes prior to IRA. > > > OK for the trunk. > > Jeff >
On 02/12/2016 02:18 PM, Jiong Wang wrote: >>> >>> PR rtl-optimization/69752 >>> * ira.c (update_equiv_regs): When looking for more than a single >>> SET, >>> also take other side effects into account. > > Will it be better that we don't remove the insn if it has side-effect > instead of don't record the equiv? > > This can offer more equiv for later rtl optimization? It's an option I considered, but for stage4 this seems like the safest fix. It's not like this is a very common problem, the effect should be negligible. Bernd
On 12/02/16 13:33, Bernd Schmidt wrote: > On 02/12/2016 02:18 PM, Jiong Wang wrote: >>>> >>>> PR rtl-optimization/69752 >>>> * ira.c (update_equiv_regs): When looking for more than a single >>>> SET, >>>> also take other side effects into account. >> >> Will it be better that we don't remove the insn if it has side-effect >> instead of don't record the equiv? >> >> This can offer more equiv for later rtl optimization? > > It's an option I considered, but for stage4 this seems like the safest > fix. It's not like this is a very common problem, the effect should be > negligible. I see, thanks for the explanation. From my quick test, this patch actually generate code slightly better. The interesting thing I found is if we still initialize the equiv, then a later use in the testcase will be rematerialized, but the equiv constant -1044200508 is forced into memory, thus the rematerialization is a load from memory. Instead, if we don't initialize the equiv then the register contains the reusable value will be kept alive. > > > Bernd >
On 02/12/2016 08:43 AM, Jeff Law wrote: > On 02/11/2016 06:28 PM, Bernd Schmidt wrote: >> PR rtl-optimization/69752 >> * ira.c (update_equiv_regs): When looking for more than a single SET, >> also take other side effects into account. > > OK for the trunk. Branches too? The problem obviously exists everywhere. Bernd
On 02/15/2016 05:38 AM, Bernd Schmidt wrote: > On 02/12/2016 08:43 AM, Jeff Law wrote: >> On 02/11/2016 06:28 PM, Bernd Schmidt wrote: > >>> PR rtl-optimization/69752 >>> * ira.c (update_equiv_regs): When looking for more than a single >>> SET, >>> also take other side effects into account. >> >> OK for the trunk. > > Branches too? The problem obviously exists everywhere. Anywhere you deem appropriate. jeff
PR rtl-optimization/69752 * ira.c (update_equiv_regs): When looking for more than a single SET, also take other side effects into account. Index: gcc/ira.c =================================================================== --- gcc/ira.c (revision 233364) +++ gcc/ira.c (working copy) @@ -3392,7 +3392,8 @@ update_equiv_regs (void) /* If this insn contains more (or less) than a single SET, only mark all destinations as having no known equivalence. */ - if (set == NULL_RTX) + if (set == NULL_RTX + || side_effects_p (SET_SRC (set))) { note_stores (PATTERN (insn), no_equiv, NULL); continue;
This seems fairly straightforward: (insn 213 455 216 6 (set (reg:SI 266) (mem/u/c:SI (post_inc:SI (reg/f:SI 267)) [4 S4 A32])) 748 {*thumb1_movsi_insn} (expr_list:REG_EQUAL (const_int -1044200508 [0xffffffffc1c2c3c4]) (expr_list:REG_INC (reg/f:SI 267) (nil)))) We don't notice that the SET_SRC has a side effect, record the insn as an equivalencing one, and later remove it because we replaced the reg with the constant everywhere. Thus, the increment doesn't take place. Fixed as follows. Bootstrapped and tested on x86_64-linux. Also compared before/after dumps for the testcase with arm-elf. Ok? Bernd