| Message ID | 8137af77d6faef78001c49d0196e475ceafc5aba.1409750557.git.segher@kernel.crashing.org |
|---|---|
| State | New |
| Headers | show |
Ping? On Wed, Sep 03, 2014 at 06:51:44AM -0700, Segher Boessenkool wrote: > This came up when investigating PR62151. In that PR combine messes up a > four-insn combination. It should really have done the combination of the > first three insns in that. The second of those instructions sets a register; > the third clobbers the same. Substituting the source of the set into the > clobber usually results in invalid RTL so that the combination will not be > valid; also, it confuses the undobuf machinery. can_combine_p rejects > the combination for this reason. > > But we can simply make subst not substitute into clobbers of registers. > Any unnecessary clobbers will be removed later anyway. > > With this patch, the three-insn combination in PR62151 is successful. It > does likely not really solve the problem there though, it just hides it. > > Bootstrapped and regression checked on powerpc64-linux, options > -m64,-m32,-m32/-mpowerpc64. > > Is this okay for mainline? > > > Segher > > > 2014-09-03 Segher Boessenkool <segher@kernel.crashing.org> > > PR rtl-optimization/62151 > * combine.c (can_combine_p): Allow the destination register of INSN > to be clobbered in I3. > (subst): Do not substitute into clobbers of registers. > > --- > gcc/combine.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/gcc/combine.c b/gcc/combine.c > index 60524b5..6a5dfbb 100644 > --- a/gcc/combine.c > +++ b/gcc/combine.c > @@ -1950,11 +1950,7 @@ can_combine_p (rtx_insn *insn, rtx_insn *i3, rtx_insn *pred ATTRIBUTE_UNUSED, > for (i = XVECLEN (PATTERN (i3), 0) - 1; i >= 0; i--) > if (GET_CODE (XVECEXP (PATTERN (i3), 0, i)) == CLOBBER) > { > - /* Don't substitute for a register intended as a clobberable > - operand. */ > rtx reg = XEXP (XVECEXP (PATTERN (i3), 0, i), 0); > - if (rtx_equal_p (reg, dest)) > - return 0; > > /* If the clobber represents an earlyclobber operand, we must not > substitute an expression containing the clobbered register. > @@ -4964,6 +4960,11 @@ subst (rtx x, rtx from, rtx to, int in_dest, int in_cond, int unique_copy) > || (REG_P (X) && REG_P (Y) \ > && REGNO (X) == REGNO (Y) && GET_MODE (X) == GET_MODE (Y))) > > + /* Do not substitute into clobbers of regs -- this will never result in > + valid RTL. */ > + if (GET_CODE (x) == CLOBBER && REG_P (XEXP (x, 0))) > + return x; > + > if (! in_dest && COMBINE_RTX_EQUAL_P (x, from)) > { > n_occurrences++; > -- > 1.8.1.4
On 09/11/14 13:01, Segher Boessenkool wrote:
> Ping?
Not forgotten. Still waiting to hear back from Bin on my recommendation
that we drop the bogus note on the floor and avoid combining pseudos
with multiple sets like that.
Jeff
On 09/03/14 07:51, Segher Boessenkool wrote: > This came up when investigating PR62151. In that PR combine messes up a > four-insn combination. It should really have done the combination of the > first three insns in that. The second of those instructions sets a register; > the third clobbers the same. Substituting the source of the set into the > clobber usually results in invalid RTL so that the combination will not be > valid; also, it confuses the undobuf machinery. can_combine_p rejects > the combination for this reason. > > But we can simply make subst not substitute into clobbers of registers. > Any unnecessary clobbers will be removed later anyway. > > With this patch, the three-insn combination in PR62151 is successful. It > does likely not really solve the problem there though, it just hides it. > > Bootstrapped and regression checked on powerpc64-linux, options > -m64,-m32,-m32/-mpowerpc64. > > Is this okay for mainline? > > > Segher > > > 2014-09-03 Segher Boessenkool <segher@kernel.crashing.org> > > PR rtl-optimization/62151 > * combine.c (can_combine_p): Allow the destination register of INSN > to be clobbered in I3. > (subst): Do not substitute into clobbers of registers. Can you add a testcase which shows the 3-insn combination from PR62151 applying? With that change, approved. jeff
On Fri, Sep 12, 2014 at 4:01 AM, Jeff Law <law@redhat.com> wrote: > On 09/11/14 13:01, Segher Boessenkool wrote: >> >> Ping? > > Not forgotten. Still waiting to hear back from Bin on my recommendation > that we drop the bogus note on the floor and avoid combining pseudos with > multiple sets like that. Sorry that I missed this message days ago. I am currently on load/store pair which has higher priority. I will re-visit this after load/store pair issue, only thing I need is to include test case from PR62151 thus I can keep track of it, just as Jeff commented. Thanks, bin
On Mon, Sep 22, 2014 at 04:20:12PM -0600, Jeff Law wrote: > Can you add a testcase which shows the 3-insn combination from PR62151 > applying? I've tried to make a stable future-proof testcase that does such a three-insn combination. Not easy at all. But now it dawns on me: do you just want the actual testcase from the PR? (Well, fixed so that it is valid C, I suppose). With a test that combine does its job, of course? Not sure how to test that, but maybe I'll learn. Or is a test showing the testcase working after the change good enough? Segher
On 09/27/14 16:03, Segher Boessenkool wrote: > On Mon, Sep 22, 2014 at 04:20:12PM -0600, Jeff Law wrote: >> Can you add a testcase which shows the 3-insn combination from PR62151 >> applying? > > I've tried to make a stable future-proof testcase that does such a three-insn > combination. Not easy at all. > > But now it dawns on me: do you just want the actual testcase from the PR? > (Well, fixed so that it is valid C, I suppose). With a test that combine > does its job, of course? Not sure how to test that, but maybe I'll learn. > > Or is a test showing the testcase working after the change good enough? It's going to be hard to totally future proof this kind of test and make it independent across every target, etc. In those cases, I just ask you do something reasonable. That can include making the test only applicable on a small number of targets and testing the RTL or assembly dumps. So, I'd use the testcase from the PR and probably scan the combine dump and probably make it target dependent. If the combine dump isn't particularly good to search for some reason, scan the assembly output. jeff
diff --git a/gcc/combine.c b/gcc/combine.c index 60524b5..6a5dfbb 100644 --- a/gcc/combine.c +++ b/gcc/combine.c @@ -1950,11 +1950,7 @@ can_combine_p (rtx_insn *insn, rtx_insn *i3, rtx_insn *pred ATTRIBUTE_UNUSED, for (i = XVECLEN (PATTERN (i3), 0) - 1; i >= 0; i--) if (GET_CODE (XVECEXP (PATTERN (i3), 0, i)) == CLOBBER) { - /* Don't substitute for a register intended as a clobberable - operand. */ rtx reg = XEXP (XVECEXP (PATTERN (i3), 0, i), 0); - if (rtx_equal_p (reg, dest)) - return 0; /* If the clobber represents an earlyclobber operand, we must not substitute an expression containing the clobbered register. @@ -4964,6 +4960,11 @@ subst (rtx x, rtx from, rtx to, int in_dest, int in_cond, int unique_copy) || (REG_P (X) && REG_P (Y) \ && REGNO (X) == REGNO (Y) && GET_MODE (X) == GET_MODE (Y))) + /* Do not substitute into clobbers of regs -- this will never result in + valid RTL. */ + if (GET_CODE (x) == CLOBBER && REG_P (XEXP (x, 0))) + return x; + if (! in_dest && COMBINE_RTX_EQUAL_P (x, from)) { n_occurrences++;