| Message ID | c79f44f2-3006-b878-9c1c-4a406f50bb99@imgtec.com |
|---|---|
| State | New |
| Headers | show |
On 12/20/2016 07:38 AM, James Cowgill wrote: > Hi, > > On 19/12/16 21:43, Jeff Law wrote: >> On 12/19/2016 08:44 AM, James Cowgill wrote: >>> 2016-12-16 James Cowgill <James.Cowgill@imgtec.com> >>> >>> PR rtl-optimization/65618 >>> * emit-rtl.c (try_split): Update "after" when moving a >>> NOTE_INSN_CALL_ARG_LOCATION. >>> >>> diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c >>> index 7de17454037..6be124ac038 100644 >>> --- a/gcc/emit-rtl.c >>> +++ b/gcc/emit-rtl.c >>> @@ -3742,6 +3742,11 @@ try_split (rtx pat, rtx_insn *trial, int last) >>> next = NEXT_INSN (next)) >>> if (NOTE_KIND (next) == NOTE_INSN_CALL_ARG_LOCATION) >>> { >>> + /* Advance after to the next instruction if it is about to >>> + be removed. */ >>> + if (after == next) >>> + after = NEXT_INSN (after); >>> + >>> remove_insn (next); >>> add_insn_after (next, insn, NULL); >>> break; >>> >> So the thing I don't like when looking at this code is we set AFTER >> immediately upon entry to try_split. But we don't use it until near the >> very end of try_split. That's just asking for trouble. >> >> Can we reasonably initialize AFTER just before it's used? > > I wasn't sure but looking closer I think that would be fine. This patch > also works and does what Richard Sandiford suggested in the PR. > > 2016-12-20 James Cowgill <James.Cowgill@imgtec.com> > > PR rtl-optimization/65618 > * emit-rtl.c (try_split): Move initialization of "before" and > "after" to just before the call to emit_insn_after_setloc. OK. jeff
On 01/01/17 22:27, Jeff Law wrote: > On 12/20/2016 07:38 AM, James Cowgill wrote: >> Hi, >> >> On 19/12/16 21:43, Jeff Law wrote: >>> On 12/19/2016 08:44 AM, James Cowgill wrote: >>>> 2016-12-16 James Cowgill <James.Cowgill@imgtec.com> >>>> >>>> PR rtl-optimization/65618 >>>> * emit-rtl.c (try_split): Update "after" when moving a >>>> NOTE_INSN_CALL_ARG_LOCATION. >>>> >>>> diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c >>>> index 7de17454037..6be124ac038 100644 >>>> --- a/gcc/emit-rtl.c >>>> +++ b/gcc/emit-rtl.c >>>> @@ -3742,6 +3742,11 @@ try_split (rtx pat, rtx_insn *trial, int last) >>>> next = NEXT_INSN (next)) >>>> if (NOTE_KIND (next) == NOTE_INSN_CALL_ARG_LOCATION) >>>> { >>>> + /* Advance after to the next instruction if it is about to >>>> + be removed. */ >>>> + if (after == next) >>>> + after = NEXT_INSN (after); >>>> + >>>> remove_insn (next); >>>> add_insn_after (next, insn, NULL); >>>> break; >>>> >>> So the thing I don't like when looking at this code is we set AFTER >>> immediately upon entry to try_split. But we don't use it until near the >>> very end of try_split. That's just asking for trouble. >>> >>> Can we reasonably initialize AFTER just before it's used? >> >> I wasn't sure but looking closer I think that would be fine. This patch >> also works and does what Richard Sandiford suggested in the PR. >> >> 2016-12-20 James Cowgill <James.Cowgill@imgtec.com> >> >> PR rtl-optimization/65618 >> * emit-rtl.c (try_split): Move initialization of "before" and >> "after" to just before the call to emit_insn_after_setloc. > OK. Great. Can you commit this for me, since I don't have commit access? Thanks, James
On 01/03/2017 04:04 AM, James Cowgill wrote: > On 01/01/17 22:27, Jeff Law wrote: >> On 12/20/2016 07:38 AM, James Cowgill wrote: >>> Hi, >>> >>> On 19/12/16 21:43, Jeff Law wrote: >>>> On 12/19/2016 08:44 AM, James Cowgill wrote: >>>>> 2016-12-16 James Cowgill <James.Cowgill@imgtec.com> >>>>> >>>>> PR rtl-optimization/65618 >>>>> * emit-rtl.c (try_split): Update "after" when moving a >>>>> NOTE_INSN_CALL_ARG_LOCATION. >>>>> >>>>> diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c >>>>> index 7de17454037..6be124ac038 100644 >>>>> --- a/gcc/emit-rtl.c >>>>> +++ b/gcc/emit-rtl.c >>>>> @@ -3742,6 +3742,11 @@ try_split (rtx pat, rtx_insn *trial, int last) >>>>> next = NEXT_INSN (next)) >>>>> if (NOTE_KIND (next) == NOTE_INSN_CALL_ARG_LOCATION) >>>>> { >>>>> + /* Advance after to the next instruction if it is about to >>>>> + be removed. */ >>>>> + if (after == next) >>>>> + after = NEXT_INSN (after); >>>>> + >>>>> remove_insn (next); >>>>> add_insn_after (next, insn, NULL); >>>>> break; >>>>> >>>> So the thing I don't like when looking at this code is we set AFTER >>>> immediately upon entry to try_split. But we don't use it until near the >>>> very end of try_split. That's just asking for trouble. >>>> >>>> Can we reasonably initialize AFTER just before it's used? >>> >>> I wasn't sure but looking closer I think that would be fine. This patch >>> also works and does what Richard Sandiford suggested in the PR. >>> >>> 2016-12-20 James Cowgill <James.Cowgill@imgtec.com> >>> >>> PR rtl-optimization/65618 >>> * emit-rtl.c (try_split): Move initialization of "before" and >>> "after" to just before the call to emit_insn_after_setloc. >> OK. > > Great. Can you commit this for me, since I don't have commit access? Done. If you're going to be contributing regularly we should probably start the process of getting you commit access. jeff
diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c index 7de17454037..bdc984c65cf 100644 --- a/gcc/emit-rtl.c +++ b/gcc/emit-rtl.c @@ -3643,8 +3643,7 @@ mark_label_nuses (rtx x) rtx_insn * try_split (rtx pat, rtx_insn *trial, int last) { - rtx_insn *before = PREV_INSN (trial); - rtx_insn *after = NEXT_INSN (trial); + rtx_insn *before, *after; rtx note; rtx_insn *seq, *tem; int probability; @@ -3818,6 +3817,9 @@ try_split (rtx pat, rtx_insn *trial, int last) } } + before = PREV_INSN (trial); + after = NEXT_INSN (trial); + tem = emit_insn_after_setloc (seq, trial, INSN_LOCATION (trial)); delete_insn (trial);