| Message ID | 6d31ccdf868ceee465d89fbc6ae80c141244f42b.1415984897.git.segher@kernel.crashing.org |
|---|---|
| State | New |
| Headers | show |
On 11/14/14 12:19, Segher Boessenkool wrote: > If the result of combining some insns is a PARALLEL of two SETs, and that > is not a recognised insn, and one of the SETs is dead, combine tries to > use the remaining SET as insn. > > This patch changes things around so that the one SET is preferably used, > not the PARALLEL. > > > 2014-11-14 Segher Boessenkool <segher@kernel.crashing.org> > > gcc/ > * combine.c (try_combine): Prefer to delete dead SETs inside > a PARALLEL over keeping them. OK. Can this go forward independent of the other changes? Seems like it should. Does it help pr52714 where we'd like to rip apart a PARALLEL with two sets, one of which is dead. If so, it might allow us to optimize that code better. Granted, it originally was an m68k issue, but presumably it's happening eleswhere or you wouldn't be poking at it :-) jeff
On Mon, Nov 17, 2014 at 02:13:22PM -0700, Jeff Law wrote: > On 11/14/14 12:19, Segher Boessenkool wrote: > >If the result of combining some insns is a PARALLEL of two SETs, and that > >is not a recognised insn, and one of the SETs is dead, combine tries to > >use the remaining SET as insn. > > > >This patch changes things around so that the one SET is preferably used, > >not the PARALLEL. > > > > > >2014-11-14 Segher Boessenkool <segher@kernel.crashing.org> > > > >gcc/ > > * combine.c (try_combine): Prefer to delete dead SETs inside > > a PARALLEL over keeping them. > OK. Can this go forward independent of the other changes? Seems like > it should. Yes, only 4 depends on 3, the rest are independent patches. > Does it help pr52714 where we'd like to rip apart a PARALLEL with two > sets, one of which is dead. If so, it might allow us to optimize that > code better. It does not seem to fix the testcase. I'll investigate why not. You're talking about the (parallel [(set (pc) (pc)) (set (X) (sp))]) right? I guess the "set pc pc" is not marked as unused... > Granted, it originally was an m68k issue, but presumably > it's happening eleswhere or you wouldn't be poking at it :-) The case that made me do this is PowerPC, where (with more patches) for long long subfM1(long long a) { return 0x1ffffffff - a; } we generated (-m32) subfic 4,4,-1 ; subfic 3,3,1 where that first subfic is (parallel [(set (reg 4) (not (reg 4))) (set (ca) (const_int 1))]) with that second set dead, so we can just do not 4,4 ; subfic 3,3,1 which is cheaper. Segher
On Tue, Nov 18, 2014 at 06:38:49AM -0600, Segher Boessenkool wrote: > > Does it help pr52714 where we'd like to rip apart a PARALLEL with two > > sets, one of which is dead. If so, it might allow us to optimize that > > code better. > > It does not seem to fix the testcase. I'll investigate why not. > You're talking about the > > (parallel [(set (pc) (pc)) > (set (X) (sp))]) > > right? I guess the "set pc pc" is not marked as unused... The very first thing that is checked for is !(added_sets_2 && i1 == 0) which matches in this case. I wonder what that condition is supposed to accomplish -- "optimisation" I suppose? Hacking that out, it still won't match because the "set pc pc" is not considered dead. It's a no-op, not the same thing. I'll try to widen the condition... Segher
On 11/18/14 06:42, Segher Boessenkool wrote: > On Tue, Nov 18, 2014 at 06:38:49AM -0600, Segher Boessenkool wrote: >>> Does it help pr52714 where we'd like to rip apart a PARALLEL with two >>> sets, one of which is dead. If so, it might allow us to optimize that >>> code better. >> >> It does not seem to fix the testcase. I'll investigate why not. >> You're talking about the >> >> (parallel [(set (pc) (pc)) >> (set (X) (sp))]) >> >> right? I guess the "set pc pc" is not marked as unused... > > The very first thing that is checked for is !(added_sets_2 && i1 == 0) > which matches in this case. I wonder what that condition is supposed > to accomplish -- "optimisation" I suppose? > > Hacking that out, it still won't match because the "set pc pc" is not > considered dead. It's a no-op, not the same thing. I'll try to widen > the condition... Just to be clear, I wouldn't consider fixing this a requirement to get your change in. I think 5/5 was ready to go as-is. Obviously if you can improve on it, that's great ;-) jeff
diff --git a/gcc/combine.c b/gcc/combine.c index 8c7f052..d9292df 100644 --- a/gcc/combine.c +++ b/gcc/combine.c @@ -3318,29 +3318,25 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn *i1, rtx_insn *i0, RTVEC_ELT (newpat_vec_with_clobbers, i) = XVECEXP (newpat, 0, i); } - /* Is the result of combination a valid instruction? */ - insn_code_number = recog_for_combine (&newpat, i3, &new_i3_notes); + /* We have recognized nothing yet. */ + insn_code_number = -1; + + /* See if this is a PARALLEL of two SETs where one SET's destination is + a register that is unused and this isn't marked as an instruction that + might trap in an EH region. In that case, we just need the other SET. + We prefer this over the PARALLEL. - /* If the result isn't valid, see if it is a PARALLEL of two SETs where - the second SET's destination is a register that is unused and isn't - marked as an instruction that might trap in an EH region. In that case, - we just need the first SET. This can occur when simplifying a divmod - insn. We *must* test for this case here because the code below that - splits two independent SETs doesn't handle this case correctly when it - updates the register status. + This can occur when simplifying a divmod insn. We *must* test for this + case here because the code below that splits two independent SETs doesn't + handle this case correctly when it updates the register status. It's pointless doing this if we originally had two sets, one from i3, and one from i2. Combining then splitting the parallel results in the original i2 again plus an invalid insn (which we delete). The net effect is only to move instructions around, which makes - debug info less accurate. + debug info less accurate. */ - Also check the case where the first SET's destination is unused. - That would not cause incorrect code, but does cause an unneeded - insn to remain. */ - - if (insn_code_number < 0 - && !(added_sets_2 && i1 == 0) + if (!(added_sets_2 && i1 == 0) && GET_CODE (newpat) == PARALLEL && XVECLEN (newpat, 0) == 2 && GET_CODE (XVECEXP (newpat, 0, 0)) == SET @@ -3349,6 +3345,7 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn *i1, rtx_insn *i0, { rtx set0 = XVECEXP (newpat, 0, 0); rtx set1 = XVECEXP (newpat, 0, 1); + rtx oldpat = newpat; if (((REG_P (SET_DEST (set1)) && find_reg_note (i3, REG_UNUSED, SET_DEST (set1))) @@ -3375,8 +3372,15 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn *i1, rtx_insn *i0, if (insn_code_number >= 0) changed_i3_dest = 1; } + + if (insn_code_number < 0) + newpat = oldpat; } + /* Is the result of combination a valid instruction? */ + if (insn_code_number < 0) + insn_code_number = recog_for_combine (&newpat, i3, &new_i3_notes); + /* If we were combining three insns and the result is a simple SET with no ASM_OPERANDS that wasn't recognized, try to split it into two insns. There are two ways to do this. It can be split using a