Re: [PATCH 2/2 v3][IRA,LRA] Fix PR86939, IRA incorrectly creates an interference between a pseudo register and a hard register

[Renlin Li <renlin dot li at foss dot arm dot com>]

Hi,

On 11/06/2018 06:58 PM, Jeff Law wrote:
> On 11/6/18 3:52 AM, Renlin Li wrote:
> > Hi Jeff & Peter,
> >
> > On 11/05/2018 07:41 PM, Jeff Law wrote:
> > > On 11/5/18 12:36 PM, Peter Bergner wrote:
> > > > On 11/5/18 1:20 PM, Jeff Law wrote:
> > > > > On 11/1/18 4:07 PM, Peter Bergner wrote:
> > > > > > On 11/1/18 1:50 PM, Renlin Li wrote:
> > > > > > > Is there any update on this issues?
> > > > > > > arm-none-linux-gnueabihf native toolchain has been mis-compiled
> > > > > > > for a while.
> > > > > >
> > > > > >   From the analysis I've done, my commit is just exposing latent issues
> > > > > > in LRA.  Can you try the patch I submitted here to see if it helps?
> > > > > >
> > > > > >     https://gcc.gnu.org/ml/gcc-patches/2018-10/msg01757.html
> > > > > >
> > > > > > It survives on powerpc64le-linux, x86_64-linux and s390x-linux.
> > > > > > Jeff threw it on his testers and said he saw an arm issue and was
> > > > > > trying to come up with a test case for me to debug.
> > > > > So I don't think the ARM issues are related to your patch, they may
> > > > > have
> > > > > been related the combiner changes that went in around the same time.
> > Yes, there are issues related to the combiner changes.
> >
> > But the IRA/LRA change dose cause the arm-none-linux-gnueabihf bootstrap
> > native toolchain mis-compiled.
> > And the new patch seems not fix this problem.
> That's strange.  I'm bootstrapping arm-linux-gnueabihf daily with qemu +
> a suitable root filesystem using Peter's most recent testing patch.
>
>
> > I am trying to extract a test case, but it is a little bit hard as the
> > toolchain itself is mis-compiled.
> > And it ICEs when compile test case with it.
> What I would suggest doing is to first start with running the testsuite
> against the stage1 compiler before/after Peter's changes.  Sometimes
> that'll turn up something useful and you can avoid debuging things
> through stage2/stage3.

Hi Jeff,
Thanks for the suggestion! I could reproduce it with stage1 compiler.



Hi Peter,

I think I found the problem!

As described in the PR, a hard register is used in
an pre/post modify expression. The hard register is live, but updated.
In this case, we should make it conflicting with all pseudos live at
that point.  Does it make sense?





It fixes the ICE of mis-compiled arm-linux-gnueabihf toolchain described in the
PR.

I attached the patch for discussion.  I haven't give a complete test on arm or
any other targets, yet. (Probably need more adjusting)

I will run arm and aarch64 regression test, cross and native.

Regards,
Renlin

BTW, The pre/post modify expression is generated by auto_inc/auto_dec pass.
somehow, it merges with hard register,  for example function argument registers.
This optimization make the life for RA harder. Probably we don't want that pass
too aggressive. @Wilco.
(This IRA/LRA and the combiner change reveals a lot of issues,
force us to work on it and improve the compiler :) .)

gcc/ChangeLog:

2018-11-08  Renlin Li  <renlin.li@arm.com>
        PR middle-end/87899
	* lra-lives.c (process_bb_lives): Make hard register of INOUT
	type conflict with all live pseudo.







> jeff
diff --git a/gcc/lra-lives.c b/gcc/lra-lives.c
index 0bf8cd06a302c8a6fcb914b94f953cdaa86597a2..370a7254cac7dbde4e320424e09274cee66c50b9 100644
--- a/gcc/lra-lives.c
+++ b/gcc/lra-lives.c
@@ -878,11 +878,25 @@ process_bb_lives (basic_block bb, int &curr_point, bool dead_insn_p)
 
       /* See which defined values die here.  */
       for (reg = curr_id->regs; reg != NULL; reg = reg->next)
-	if (reg->type == OP_OUT
-	    && ! reg_early_clobber_p (reg, n_alt) && ! reg->subreg_p)
-	  need_curr_point_incr
-	    |= mark_regno_dead (reg->regno, reg->biggest_mode,
-				curr_point);
+	if (! reg_early_clobber_p (reg, n_alt) && ! reg->subreg_p)
+	  {
+	    if (reg->type == OP_OUT)
+	      need_curr_point_incr
+		|= mark_regno_dead (reg->regno, reg->biggest_mode,
+				    curr_point);
+
+	    // This is a hard register, and it must be live.  Keep it live and
+	    // make it conflict with all live pseudo registers.
+	    else if (reg->type == OP_INOUT && reg->regno < FIRST_PSEUDO_REGISTER)
+	      {
+		lra_assert (TEST_HARD_REG_BIT (hard_regs_live, reg->regno));
+
+		unsigned int i;
+		EXECUTE_IF_SET_IN_SPARSESET (pseudos_live, i)
+		  SET_HARD_REG_BIT (lra_reg_info[i].conflict_hard_regs,
+				    reg->regno);
+	      }
+	  }
 
       for (reg = curr_static_id->hard_regs; reg != NULL; reg = reg->next)
 	if (reg->type == OP_OUT