| Message ID | f1316eb793d3a8a4f0e9302a7f33adaece291a43.1415984897.git.segher@kernel.crashing.org |
|---|---|
| State | New |
| Headers | show |
On 11/14/2014 08:19 PM, Segher Boessenkool wrote: > + /* If I2 is a PARALLEL of two SETs of REGs (and perhaps some CLOBBERs), > + make those two SETs separate I1 and I2 insns, and make an I0 that is > + the original I1. */ > + if (i0 == 0 > + && GET_CODE (PATTERN (i2)) == PARALLEL > + && XVECLEN (PATTERN (i2), 0) >= 2 > + && GET_CODE (XVECEXP (PATTERN (i2), 0, 0)) == SET > + && GET_CODE (XVECEXP (PATTERN (i2), 0, 1)) == SET > + && REG_P (SET_DEST (XVECEXP (PATTERN (i2), 0, 0))) > + && REG_P (SET_DEST (XVECEXP (PATTERN (i2), 0, 1))) > + && !reg_used_between_p (SET_DEST (XVECEXP (PATTERN (i2), 0, 0)), i2, i3) > + && !reg_used_between_p (SET_DEST (XVECEXP (PATTERN (i2), 0, 1)), i2, i3) Don't we have other code in combine checking the reg_used_between case? > + && (XVECLEN (PATTERN (i2), 0) == 2 > + || GET_CODE (XVECEXP (PATTERN (i2), 0, 2)) == CLOBBER)) This probably wants to test for XVECLEN == 3 for the second case. Can then drop the earlier test. I think you also need to check that !reg_overlap_mentioned_p between the two dests and the other set's sources. Bernd
On Fri, Nov 14, 2014 at 08:35:48PM +0100, Bernd Schmidt wrote: > On 11/14/2014 08:19 PM, Segher Boessenkool wrote: > >+ /* If I2 is a PARALLEL of two SETs of REGs (and perhaps some CLOBBERs), > >+ make those two SETs separate I1 and I2 insns, and make an I0 that is > >+ the original I1. */ > >+ if (i0 == 0 > >+ && GET_CODE (PATTERN (i2)) == PARALLEL > >+ && XVECLEN (PATTERN (i2), 0) >= 2 > >+ && GET_CODE (XVECEXP (PATTERN (i2), 0, 0)) == SET > >+ && GET_CODE (XVECEXP (PATTERN (i2), 0, 1)) == SET > >+ && REG_P (SET_DEST (XVECEXP (PATTERN (i2), 0, 0))) > >+ && REG_P (SET_DEST (XVECEXP (PATTERN (i2), 0, 1))) > >+ && !reg_used_between_p (SET_DEST (XVECEXP (PATTERN (i2), 0, 0)), > >i2, i3) > >+ && !reg_used_between_p (SET_DEST (XVECEXP (PATTERN (i2), 0, 1)), > >i2, i3) > > Don't we have other code in combine checking the reg_used_between case? It doesn't make any sense at all. What I wanted to check is whether splitting the parallel creates a conflict, but I woefully failed at that. Will fix. > >+ && (XVECLEN (PATTERN (i2), 0) == 2 > >+ || GET_CODE (XVECEXP (PATTERN (i2), 0, 2)) == CLOBBER)) > > This probably wants to test for XVECLEN == 3 for the second case. Can > then drop the earlier test. It needs to test there are exactly two SETs, any amount of clobbers, and nothing else. > I think you also need to check that !reg_overlap_mentioned_p between the > two dests and the other set's sources. Only the dest of the new I1 with the sources of the new I2, but yes. Segher
On 11/14/14 12:19, Segher Boessenkool wrote: > If I2 is a PARALLEL of two SETs, split it into two instructions, I1 > and I2. If there already was an I1, rename it to I0. If there > already was an I0, don't do anything. > > This surprisingly simple patch is enough to let combine handle such > PARALLELs properly. It's clever. > > > 2014-11-14 Segher Boessenkool <segher@kernel.crashing.org> > > gcc/ > * combine.c (try_combine): If I2 is a PARALLEL of two SETs, > split it into two insns. So you're virtually serializing the PARALLEL to make combine happy if I'm reading this correctly. THe first thing I worry about is preserving the semantics of a PARALLEL. Specifically that all the inputs are evaluated, then all the side effects happen. So I think one of the checks you need is that the destinations of the SETs are not used as source operands in the SETs. The second thing I worry about handling of match_dup operands. But presumably all the resulting insns must match in one way or another or the whole thing gets reset to its prior state. So I suspect those are OK as well. Related, I was worried about RTL structure sharing, but in the end I think those are a non-concern for the same basic reasons as match_dups aren't a real worry. > > --- > gcc/combine.c | 31 +++++++++++++++++++++++++++++++ > 1 file changed, 31 insertions(+) > > diff --git a/gcc/combine.c b/gcc/combine.c > index f7797e7..c4d23e3 100644 > --- a/gcc/combine.c > +++ b/gcc/combine.c > @@ -2780,6 +2780,37 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn *i1, rtx_insn *i0, > SUBST_LINK (LOG_LINKS (i2), alloc_insn_link (i1, LOG_LINKS (i2))); > } > } > + > + /* If I2 is a PARALLEL of two SETs of REGs (and perhaps some CLOBBERs), > + make those two SETs separate I1 and I2 insns, and make an I0 that is > + the original I1. */ > + if (i0 == 0 Test for NULL. > + && GET_CODE (PATTERN (i2)) == PARALLEL > + && XVECLEN (PATTERN (i2), 0) >= 2 > + && GET_CODE (XVECEXP (PATTERN (i2), 0, 0)) == SET > + && GET_CODE (XVECEXP (PATTERN (i2), 0, 1)) == SET > + && REG_P (SET_DEST (XVECEXP (PATTERN (i2), 0, 0))) > + && REG_P (SET_DEST (XVECEXP (PATTERN (i2), 0, 1))) > + && !reg_used_between_p (SET_DEST (XVECEXP (PATTERN (i2), 0, 0)), i2, i3) > + && !reg_used_between_p (SET_DEST (XVECEXP (PATTERN (i2), 0, 1)), i2, i3) > + && (XVECLEN (PATTERN (i2), 0) == 2 > + || GET_CODE (XVECEXP (PATTERN (i2), 0, 2)) == CLOBBER)) As noted above, I think you need to verify the set/clobbered operands do not conflict with any of the source operands. Otherwise you run the risk of changing the semantics when you rip apart the PARALLEL. Ah, just saw that Bernd made the same observation. Good. And I think while convention has CLOBBERs at the end of insns, I don't think that's a hard requirement. So I think you need a stronger check for elements 2 and beyond in the vector. OK with the direction this is going, but I think another iteration is going to be necessary. Jeff
On Tue, Nov 25, 2014 at 11:35:14AM -0700, Jeff Law wrote: > On 11/14/14 12:19, Segher Boessenkool wrote: > >If I2 is a PARALLEL of two SETs, split it into two instructions, I1 > >and I2. If there already was an I1, rename it to I0. If there > >already was an I0, don't do anything. > > > >This surprisingly simple patch is enough to let combine handle such > >PARALLELs properly. > It's clever. It does a very similar thing as the code right before it. > So you're virtually serializing the PARALLEL to make combine happy if > I'm reading this correctly. That is correct. > THe first thing I worry about is preserving the semantics of a PARALLEL. > Specifically that all the inputs are evaluated, then all the side > effects happen. So I think one of the checks you need is that the > destinations of the SETs are not used as source operands in the SETs. As you say below, Bernd also noticed this; I haven't found the time to make a new patch yet. "Soon". > The second thing I worry about handling of match_dup operands. But > presumably all the resulting insns must match in one way or another or > the whole thing gets reset to its prior state. So I suspect those are > OK as well. > > Related, I was worried about RTL structure sharing, but in the end I > think those are a non-concern for the same basic reasons as match_dups > aren't a real worry. combine make a copy of any RTL before it modifies it. > >+ /* If I2 is a PARALLEL of two SETs of REGs (and perhaps some CLOBBERs), > >+ make those two SETs separate I1 and I2 insns, and make an I0 that is > >+ the original I1. */ > >+ if (i0 == 0 > Test for NULL. All similar code in combine tests for 0. I'd have written "if (!i0)" otherwise. > And I think while convention has CLOBBERs at the end of insns, I don't > think that's a hard requirement. So I think you need a stronger check > for elements 2 and beyond in the vector. I'll check if 0,1 are SETs and all of 2..N-1 are CLOBBERs. I was thinking there could be nothing else but SETs and CLOBBERs (which combine already does expect to always be in that order), but let's be less tricky. > OK with the direction this is going, but I think another iteration is > going to be necessary. Great, new patch coming up. Segher
On Sat, Nov 15, 2014 at 06:59:23AM -0600, Segher Boessenkool wrote: > On Fri, Nov 14, 2014 at 08:35:48PM +0100, Bernd Schmidt wrote: > > On 11/14/2014 08:19 PM, Segher Boessenkool wrote: > > >+ /* If I2 is a PARALLEL of two SETs of REGs (and perhaps some CLOBBERs), > > >+ make those two SETs separate I1 and I2 insns, and make an I0 that is > > >+ the original I1. */ > > >+ if (i0 == 0 > > >+ && GET_CODE (PATTERN (i2)) == PARALLEL > > >+ && XVECLEN (PATTERN (i2), 0) >= 2 > > >+ && GET_CODE (XVECEXP (PATTERN (i2), 0, 0)) == SET > > >+ && GET_CODE (XVECEXP (PATTERN (i2), 0, 1)) == SET > > >+ && REG_P (SET_DEST (XVECEXP (PATTERN (i2), 0, 0))) > > >+ && REG_P (SET_DEST (XVECEXP (PATTERN (i2), 0, 1))) > > >+ && !reg_used_between_p (SET_DEST (XVECEXP (PATTERN (i2), 0, 0)), > > >i2, i3) > > >+ && !reg_used_between_p (SET_DEST (XVECEXP (PATTERN (i2), 0, 1)), > > >i2, i3) > > > > Don't we have other code in combine checking the reg_used_between case? Actually, no, no we don't. Under the old regime (before adding the regno field to log_links), a link links a first insn with its earliest successor that uses any of the regs that first insn sets. This guarantees that in try_combine none of the insns between I2 and I3 will use any of the registers set by I2. Under the new regime (patches 3 and 4) a link links a first insn with its earliest successor that uses a particular reg the first insn sets. For single sets, this is the same as before; but not so for multiple sets. There are only two cases where this matters: this patch, and the code right before it (that never seems to trigger, not on any target; trying to figure out if that is true, and if so, why). New patchset is bootstrapping/testing. Cheers, Segher
diff --git a/gcc/combine.c b/gcc/combine.c index f7797e7..c4d23e3 100644 --- a/gcc/combine.c +++ b/gcc/combine.c @@ -2780,6 +2780,37 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn *i1, rtx_insn *i0, SUBST_LINK (LOG_LINKS (i2), alloc_insn_link (i1, LOG_LINKS (i2))); } } + + /* If I2 is a PARALLEL of two SETs of REGs (and perhaps some CLOBBERs), + make those two SETs separate I1 and I2 insns, and make an I0 that is + the original I1. */ + if (i0 == 0 + && GET_CODE (PATTERN (i2)) == PARALLEL + && XVECLEN (PATTERN (i2), 0) >= 2 + && GET_CODE (XVECEXP (PATTERN (i2), 0, 0)) == SET + && GET_CODE (XVECEXP (PATTERN (i2), 0, 1)) == SET + && REG_P (SET_DEST (XVECEXP (PATTERN (i2), 0, 0))) + && REG_P (SET_DEST (XVECEXP (PATTERN (i2), 0, 1))) + && !reg_used_between_p (SET_DEST (XVECEXP (PATTERN (i2), 0, 0)), i2, i3) + && !reg_used_between_p (SET_DEST (XVECEXP (PATTERN (i2), 0, 1)), i2, i3) + && (XVECLEN (PATTERN (i2), 0) == 2 + || GET_CODE (XVECEXP (PATTERN (i2), 0, 2)) == CLOBBER)) + { + /* If there is no I1, there is no I0 either. */ + i0 = i1; + + /* We make I1 with the same INSN_UID as I2. This gives it + the same DF_INSN_LUID for value tracking. Our fake I1 will + never appear in the insn stream so giving it the same INSN_UID + as I2 will not cause a problem. */ + + i1 = gen_rtx_INSN (VOIDmode, NULL, i2, BLOCK_FOR_INSN (i2), + XVECEXP (PATTERN (i2), 0, 0), INSN_LOCATION (i2), + -1, NULL_RTX); + INSN_UID (i1) = INSN_UID (i2); + + SUBST (PATTERN (i2), XVECEXP (PATTERN (i2), 0, 1)); + } #endif /* Verify that I2 and I1 are valid for combining. */