[PATCH] [LRA] Fix wrong-code PR 91109

[PATCH] [LRA] Fix wrong-code PR 91109

[Bernd Edlinger]

[-- Attachment #1: Type: text/plain, Size: 831 bytes --]

Hi!


PR 91109 is a wrong-code bug, where LRA is using a scratch register
which is actually not available for use, and thus gets clobbered
when it should not.  That seems to be mostly because the live range
info of the cloned schatch register is not working the way how update_scrach_ops
sets up the new register.  Moreover for the new register there is
a much better alternative free register available, so that just not
trying the re-use the previous hard register assignment solves the problem.

For more background please see the bugzilla PR 91109.

Since I am able to reproduce this bug with latest gcc-9 branch, I want
to ask right away, if it is okay to back-port after a while.


Boot-strapped and reg-tested on x86_64-pc-linux-gnu and armv7-linux-gnueabihf.
Is it OK for trunk?


Thanks,
Bernd.



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch-pr91109.diff --]
[-- Type: text/x-patch; name="patch-pr91109.diff", Size: 1261 bytes --]

2019-07-30  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	PR tree-optimization/91109
	* lra-remat.c (update_scratch_ops): Remove assignment of the
	hard register.

Index: gcc/lra-remat.c
===================================================================
--- gcc/lra-remat.c	(revision 273767)
+++ gcc/lra-remat.c	(working copy)
@@ -1021,7 +1021,6 @@ get_hard_regs (struct lra_insn_reg *reg, int &nreg
 static void
 update_scratch_ops (rtx_insn *remat_insn)
 {
-  int hard_regno;
   lra_insn_recog_data_t id = lra_get_insn_recog_data (remat_insn);
   struct lra_static_insn_data *static_id = id->insn_static_data;
   for (int i = 0; i < static_id->n_operands; i++)
@@ -1032,17 +1031,9 @@ update_scratch_ops (rtx_insn *remat_insn)
       int regno = REGNO (*loc);
       if (! lra_former_scratch_p (regno))
 	continue;
-      hard_regno = reg_renumber[regno];
       *loc = lra_create_new_reg (GET_MODE (*loc), *loc,
 				 lra_get_allocno_class (regno),
 				 "scratch pseudo copy");
-      if (hard_regno >= 0)
-	{
-	  reg_renumber[REGNO (*loc)] = hard_regno;
-	  if (lra_dump_file)
-	    fprintf (lra_dump_file, "	 Assigning the same %d to r%d\n",
-		     REGNO (*loc), hard_regno);
-	}
       lra_register_new_scratch_op (remat_insn, i, id->icode);
     }
   

Re: [PATCH] [LRA] Fix wrong-code PR 91109

[Vladimir Makarov]

On 8/5/19 4:37 PM, Bernd Edlinger wrote:
> Hi!
>
>
> PR 91109 is a wrong-code bug, where LRA is using a scratch register
> which is actually not available for use, and thus gets clobbered
> when it should not.  That seems to be mostly because the live range
> info of the cloned schatch register is not working the way how update_scrach_ops
> sets up the new register.  Moreover for the new register there is
> a much better alternative free register available, so that just not
> trying the re-use the previous hard register assignment solves the problem.
>
> For more background please see the bugzilla PR 91109.
>
> Since I am able to reproduce this bug with latest gcc-9 branch, I want
> to ask right away, if it is okay to back-port after a while.
>
>
> Boot-strapped and reg-tested on x86_64-pc-linux-gnu and armv7-linux-gnueabihf.
> Is it OK for trunk?

Thank you for working on the problem which is severe as the wrong code 
is generated.  The patch is ok as an intermediate solution. You can 
commit it to the trunk and gcc-9 branch.

Still I think more work on the PR is needed.  If subsequent LRA sub-pass 
spills some pseudo to assign a hard register to the scratch of the 
rematerialized insn as it was in the original insn, it might make this 
rematerialization unprofitable.  So I'll think how to avoid the 
unprofitable rematerialization in such cases and would like to work on 
this  PR more.

Please, do not close the PR after committing the patch.  I am going to 
work on it more when stage3 starts.

Re: [PATCH] [LRA] Fix wrong-code PR 91109

[Bernd Edlinger]

Hi Jakub,

I think this wrong code bug would be good to be fixed in 9.2.

Would you like me to go ahead, or should it wait for 9.3 ?

Thanks
Bernd.


On 8/7/19 3:32 PM, Vladimir Makarov wrote:
> On 8/5/19 4:37 PM, Bernd Edlinger wrote:
>> Hi!
>>
>>
>> PR 91109 is a wrong-code bug, where LRA is using a scratch register
>> which is actually not available for use, and thus gets clobbered
>> when it should not.  That seems to be mostly because the live range
>> info of the cloned schatch register is not working the way how update_scrach_ops
>> sets up the new register.  Moreover for the new register there is
>> a much better alternative free register available, so that just not
>> trying the re-use the previous hard register assignment solves the problem.
>>
>> For more background please see the bugzilla PR 91109.
>>
>> Since I am able to reproduce this bug with latest gcc-9 branch, I want
>> to ask right away, if it is okay to back-port after a while.
>>
>>
>> Boot-strapped and reg-tested on x86_64-pc-linux-gnu and armv7-linux-gnueabihf.
>> Is it OK for trunk?
> 
> Thank you for working on the problem which is severe as the wrong code is generated.  The patch is ok as an intermediate solution. You can commit it to the trunk and gcc-9 branch.
> 
> Still I think more work on the PR is needed.  If subsequent LRA sub-pass spills some pseudo to assign a hard register to the scratch of the rematerialized insn as it was in the original insn, it might make this rematerialization unprofitable.  So I'll think how to avoid the unprofitable rematerialization in such cases and would like to work on this  PR more.
> 
> Please, do not close the PR after committing the patch.  I am going to work on it more when stage3 starts.
> 
> 

Re: [PATCH] [LRA] Fix wrong-code PR 91109

[Jakub Jelinek]

On Fri, Aug 09, 2019 at 10:31:57AM +0000, Bernd Edlinger wrote:
> I think this wrong code bug would be good to be fixed in 9.2.
> 
> Would you like me to go ahead, or should it wait for 9.3 ?

Wait for 9.2.1 reopening, even if we'd roll another RC, I'd be afraid that
for RA changes, especially ones that aren't a severe recent regression like
this, there is not enough time to have it tested enough.

	Jakub

[PATCH] [LRA] Fix wrong-code PR 91109 take 2

[Bernd Edlinger]

[-- Attachment #1: Type: text/plain, Size: 489 bytes --]

Hi,

as discussed in the PR 91109 audit trail,
my previous patch missed a case where no spilling is necessary,
but the re-materialized instruction has now scratch regs without
a hard register assignment.  And thus the LRA pass falls out of
the loop pre-maturely.

Fixed by checking for scratch regs with no assignment
and continuing the loop in that case.


Boot-strapped and reg-tested on x86_64-pc-linux-gnu and arm-linux-gnueabihf.
Is it OK for trunk?


Thanks
Bernd.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch-pr91109-1.diff --]
[-- Type: text/x-patch; name="patch-pr91109-1.diff", Size: 1795 bytes --]

2019-08-12  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	PR tree-optimization/91109
	* lra-int.h (lra_need_for_scratch_reg_p): Declare.
	* lra.c (lra): Use lra_need_for_scratch_reg_p.
	* lra-spills.c (lra_need_for_scratch_reg_p): New function.

Index: gcc/lra-int.h
===================================================================
--- gcc/lra-int.h	(revision 274168)
+++ gcc/lra-int.h	(working copy)
@@ -396,6 +396,7 @@ extern bool lra_coalesce (void);
 
 /* lra-spills.c:  */
 
+extern bool lra_need_for_scratch_reg_p (void);
 extern bool lra_need_for_spills_p (void);
 extern void lra_spill (void);
 extern void lra_final_code_change (void);
Index: gcc/lra-spills.c
===================================================================
--- gcc/lra-spills.c	(revision 274168)
+++ gcc/lra-spills.c	(working copy)
@@ -549,6 +549,19 @@ spill_pseudos (void)
     }
 }
 
+/* Return true if we need scratch reg assignments.  */
+bool
+lra_need_for_scratch_reg_p (void)
+{
+  int i; max_regno = max_reg_num ();
+
+  for (i = FIRST_PSEUDO_REGISTER; i < max_regno; i++)
+    if (lra_reg_info[i].nrefs != 0 && lra_get_regno_hard_regno (i) < 0
+	&& lra_former_scratch_p (i))
+      return true;
+  return false;
+}
+
 /* Return true if we need to change some pseudos into memory.  */
 bool
 lra_need_for_spills_p (void)
Index: gcc/lra.c
===================================================================
--- gcc/lra.c	(revision 274168)
+++ gcc/lra.c	(working copy)
@@ -2567,7 +2567,11 @@ lra (FILE *f)
 	  lra_create_live_ranges (lra_reg_spill_p, true);
 	  live_p = true;
 	  if (! lra_need_for_spills_p ())
-	    break;
+	    {
+	      if (lra_need_for_scratch_reg_p ())
+		continue;
+	      break;
+	    }
 	}
       lra_spill ();
       /* Assignment of stack slots changes elimination offsets for

Re: [PATCH] [LRA] Fix wrong-code PR 91109 take 2

[Bernd Edlinger]

On 8/15/19 9:46 PM, Bernd Edlinger wrote:
> Hi,
> 
> as discussed in the PR 91109 audit trail,
> my previous patch missed a case where no spilling is necessary,
> but the re-materialized instruction has now scratch regs without
> a hard register assignment.  And thus the LRA pass falls out of
> the loop pre-maturely.
> 
> Fixed by checking for scratch regs with no assignment
> and continuing the loop in that case.
> 
> 
> Boot-strapped and reg-tested on x86_64-pc-linux-gnu and arm-linux-gnueabihf.
> Is it OK for trunk?
> 

Aehm, sorry, I forgot to ask, but is it also OK for the gcc-9 branch ?

Thanks
Bernd.

Re: [PATCH] [LRA] Fix wrong-code PR 91109 take 2

[Vladimir Makarov]

On 2019-08-15 3:46 p.m., Bernd Edlinger wrote:
> Hi,
>
> as discussed in the PR 91109 audit trail,
> my previous patch missed a case where no spilling is necessary,
> but the re-materialized instruction has now scratch regs without
> a hard register assignment.  And thus the LRA pass falls out of
> the loop pre-maturely.
>
> Fixed by checking for scratch regs with no assignment
> and continuing the loop in that case.
>
>
> Boot-strapped and reg-tested on x86_64-pc-linux-gnu and arm-linux-gnueabihf.
> Is it OK for trunk?

Sorry, I am afraid this patch can make LRA cycle forever in some cases.

The reason for this is an existing pattern (scratch "r,X").  So if LRA 
makes a choice for the 2nd alternative, it will be a former spilled 
scratch (such spilled pseudo is changed into scratch at the end of 
LRA).  In this case the constraint subpass satisfies all constraints. 
There are no changes at all but because there are spilled (not in remat 
subpass) former scratches we continue the loop.

I guess you need something more accurate interaction with remat subpass.

Re: [PATCH] [LRA] Fix wrong-code PR 91109 take 2

[Vladimir Makarov]

On 2019-08-16 11:06 a.m., Vladimir Makarov wrote:
>
> On 2019-08-15 3:46 p.m., Bernd Edlinger wrote:
>> Hi,
>>
>> as discussed in the PR 91109 audit trail,
>> my previous patch missed a case where no spilling is necessary,
>> but the re-materialized instruction has now scratch regs without
>> a hard register assignment.  And thus the LRA pass falls out of
>> the loop pre-maturely.
>>
>> Fixed by checking for scratch regs with no assignment
>> and continuing the loop in that case.
>>
>>
>> Boot-strapped and reg-tested on x86_64-pc-linux-gnu and 
>> arm-linux-gnueabihf.
>> Is it OK for trunk?
>
> Sorry, I am afraid this patch can make LRA cycle forever in some cases.
>
> The reason for this is an existing pattern (scratch "r,X").  So if LRA 
> makes a choice for the 2nd alternative, it will be a former spilled 
> scratch (such spilled pseudo is changed into scratch at the end of 
> LRA).  In this case the constraint subpass satisfies all constraints. 
> There are no changes at all but because there are spilled (not in 
> remat subpass) former scratches we continue the loop.
>
> I guess you need something more accurate interaction with remat subpass.
>
>
Sorry, I missed that your new code is run only if remat returns true 
which means some changes in it.  It was not seen in the patch context.  
So there will be no cycling as remat at some point stop to do changes.

The patch is ok for trunk and gcc-9 branch.

Thank you, Bernd