diff mbox

Combiner fix for PR79910

Message ID 20170318162356.GY4402@gate.crashing.org
State New
Headers show

Commit Message

Segher Boessenkool March 18, 2017, 4:23 p.m. UTC 
On Fri, Mar 17, 2017 at 04:23:57PM -0600, Jeff Law wrote:
> >>+  /* For combinations that may result in two insns, we have to gather
> >>+     some extra information about registers used, so that we can
> >>+     update all relevant LOG_LINKS later.  */
> >
> >Please just refuse to do the combination in such cases instead.
> ?!? That would essentially mean we can't do 3->2 combinations.

No, only those that would lead to trouble.

> >So, no, I'm not okay with this.  It is very expensive, it is doing open
> >heart surgery on combine's internal structures (in a way that may or may
> >not work), and all that to combine some insns in a case that should not
> >exist anyway.
> Please don't be dramatic.  If you've got a better suggestion for how to 
> fix this, be my guest.  I don't like the compile-time cost either, but I 
> don't see a better solution.

I'll commit the following monday or so:


Subject: [PATCH] combine: Fix 79910

If the dest of an I0 or I1 is used in an insn before I2, as can happen
in various uncommon cases, and we manage to do the combination, the set
is moved to I2, which is wrong.  Don't allow combining the insns in this
case.

This fixes the PR79910 testcase.  I also tested building Linux kernels
for all supported architectures: this fixes a bug for blackfin and
makes no changes on other architectures.

Bootstrapped and regression checked on x86_64-linux and powerpc64-linux
{-m32,-m64}.


Segher


2017-03-18  Segher Boessenkool  <segher@kernel.crashing.org>

	PR rtl-optimization/79910
	* combine.c (can_combine_p): Do not allow combining an I0 or I1
	if its dest is used by an insn before I2 (other than the combined
	insns themselves, which are properly handled already).

---
 gcc/combine.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Segher Boessenkool March 19, 2017, 2:07 a.m. UTC | #1 
On Sat, Mar 18, 2017 at 11:23:56AM -0500, Segher Boessenkool wrote:
> Bootstrapped and regression checked on x86_64-linux and powerpc64-linux
> {-m32,-m64}.

Now also tested on aarch64-linux; no new failures.

Segher


> 2017-03-18  Segher Boessenkool  <segher@kernel.crashing.org>
> 
> 	PR rtl-optimization/79910
> 	* combine.c (can_combine_p): Do not allow combining an I0 or I1
> 	if its dest is used by an insn before I2 (other than the combined
> 	insns themselves, which are properly handled already).
Segher Boessenkool March 21, 2017, 12:29 a.m. UTC | #2 
On Sat, Mar 18, 2017 at 11:23:56AM -0500, Segher Boessenkool wrote:
> > >So, no, I'm not okay with this.  It is very expensive, it is doing open
> > >heart surgery on combine's internal structures (in a way that may or may
> > >not work), and all that to combine some insns in a case that should not
> > >exist anyway.
> > Please don't be dramatic.  If you've got a better suggestion for how to 
> > fix this, be my guest.  I don't like the compile-time cost either, but I 
> > don't see a better solution.
> 
> I'll commit the following monday or so:

Done now, with the previous patches to combine.c reverted.


Segher
diff mbox

Patch

diff --git a/gcc/combine.c b/gcc/combine.c
index 3e5c439..24ecedf 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -1953,6 +1953,10 @@  can_combine_p (rtx_insn *insn, rtx_insn *i3, rtx_insn *pred ATTRIBUTE_UNUSED,
       || (succ2 && FIND_REG_INC_NOTE (succ2, dest))
       /* Don't substitute into a non-local goto, this confuses CFG.  */
       || (JUMP_P (i3) && find_reg_note (i3, REG_NON_LOCAL_GOTO, NULL_RTX))
+      /* Make sure that DEST is not used after INSN but before SUCC, or
+	 between SUCC and SUCC2.  */
+      || (succ && reg_used_between_p (dest, insn, succ))
+      || (succ2 && reg_used_between_p (dest, succ, succ2))
       /* Make sure that DEST is not used after SUCC but before I3.  */
       || (!all_adjacent
 	  && ((succ2