diff mbox

fix regrename pass to ensure renamings produce valid insns

Message ID 563C8580.8060805@t-online.de
State New
Headers show

Commit Message

Bernd Schmidt Nov. 6, 2015, 10:48 a.m. UTC
On 06/17/2015 07:11 PM, Sandra Loosemore wrote:

> Index: gcc/regrename.c
> ===================================================================
> --- gcc/regrename.c	(revision 224532)
> +++ gcc/regrename.c	(working copy)
> @@ -942,19 +942,22 @@ regrename_do_replace (struct du_head *he
>         int reg_ptr = REG_POINTER (*chain->loc);
>
>         if (DEBUG_INSN_P (chain->insn) && REGNO (*chain->loc) != base_regno)
> -	INSN_VAR_LOCATION_LOC (chain->insn) = gen_rtx_UNKNOWN_VAR_LOC ();
> +	validate_change (chain->insn, &(INSN_VAR_LOCATION_LOC (chain->insn)),
> +			 gen_rtx_UNKNOWN_VAR_LOC (), true);
>         else
>   	{
> -	  *chain->loc = gen_raw_REG (GET_MODE (*chain->loc), reg);
> +	  validate_change (chain->insn, chain->loc,
> +			   gen_raw_REG (GET_MODE (*chain->loc), reg), true);
>   	  if (regno >= FIRST_PSEUDO_REGISTER)
>   	    ORIGINAL_REGNO (*chain->loc) = regno;
>   	  REG_ATTRS (*chain->loc) = attr;

With a patch I'm working on that uses the renamer more often, I found 
that this is causing compare-debug failures. Validating changes to 
debug_insns (the INSN_VAR_LOCATION_LOC in particular) can apparently fail.

The following fix was bootstrapped and tested with -frename-registers 
enabled at -O1 on x86_64-linux. Ok?


Bernd

Comments

Jeff Law Nov. 6, 2015, 7:51 p.m. UTC | #1
On 11/06/2015 03:48 AM, Bernd Schmidt wrote:
> On 06/17/2015 07:11 PM, Sandra Loosemore wrote:
>
>> Index: gcc/regrename.c
>> ===================================================================
>> --- gcc/regrename.c    (revision 224532)
>> +++ gcc/regrename.c    (working copy)
>> @@ -942,19 +942,22 @@ regrename_do_replace (struct du_head *he
>>         int reg_ptr = REG_POINTER (*chain->loc);
>>
>>         if (DEBUG_INSN_P (chain->insn) && REGNO (*chain->loc) !=
>> base_regno)
>> -    INSN_VAR_LOCATION_LOC (chain->insn) = gen_rtx_UNKNOWN_VAR_LOC ();
>> +    validate_change (chain->insn, &(INSN_VAR_LOCATION_LOC
>> (chain->insn)),
>> +             gen_rtx_UNKNOWN_VAR_LOC (), true);
>>         else
>>       {
>> -      *chain->loc = gen_raw_REG (GET_MODE (*chain->loc), reg);
>> +      validate_change (chain->insn, chain->loc,
>> +               gen_raw_REG (GET_MODE (*chain->loc), reg), true);
>>         if (regno >= FIRST_PSEUDO_REGISTER)
>>           ORIGINAL_REGNO (*chain->loc) = regno;
>>         REG_ATTRS (*chain->loc) = attr;
>
> With a patch I'm working on that uses the renamer more often, I found
> that this is causing compare-debug failures. Validating changes to
> debug_insns (the INSN_VAR_LOCATION_LOC in particular) can apparently fail.
>
> The following fix was bootstrapped and tested with -frename-registers
> enabled at -O1 on x86_64-linux. Ok?
Essentially we're keeping the validate_change that Sandra & Chung added 
on the real insns to address the problem on the nios2 port.  The patch 
just drops the DEBUG_INSN changes from the change group.

For the DEBUG_INSNS in the chain, we update those independently after we 
apply the change group.

I think the change is fine for the trunk, though I'm still curious about 
how the code as-is resulted in a comparison failure.

jeff
Bernd Schmidt Nov. 13, 2015, 3:13 p.m. UTC | #2
On 11/06/2015 08:51 PM, Jeff Law wrote:
> I think the change is fine for the trunk, though I'm still curious about
> how the code as-is resulted in a comparison failure.

I've been retesting and I think this was a case of something else 
triggering an random failure - the patch made it go away on the testcase 
I looked at, but random compare-debug failures persisted. I think I have 
those fixed now. I'll leave this patch out for now.


Bernd
diff mbox

Patch

	* regrename.c (regrename_do_replace): Do not validate changes to
	debug insns.

Index: gcc/regrename.c
===================================================================
--- gcc/regrename.c	(revision 229049)
+++ gcc/regrename.c	(working copy)
@@ -946,10 +951,7 @@  regrename_do_replace (struct du_head *he
       struct reg_attrs *attr = REG_ATTRS (*chain->loc);
       int reg_ptr = REG_POINTER (*chain->loc);
 
-      if (DEBUG_INSN_P (chain->insn) && REGNO (*chain->loc) != base_regno)
-	validate_change (chain->insn, &(INSN_VAR_LOCATION_LOC (chain->insn)),
-			 gen_rtx_UNKNOWN_VAR_LOC (), true);
-      else
+      if (!DEBUG_INSN_P (chain->insn))
 	{
 	  validate_change (chain->insn, chain->loc, 
 			   gen_raw_REG (GET_MODE (*chain->loc), reg), true);
@@ -963,6 +965,16 @@  regrename_do_replace (struct du_head *he
   if (!apply_change_group ())
     return false;
 
+  for (chain = head->first; chain; chain = chain->next_use)
+    if (DEBUG_INSN_P (chain->insn))
+      {
+	if (REGNO (*chain->loc) != base_regno)
+	  INSN_VAR_LOCATION_LOC (chain->insn) = gen_rtx_UNKNOWN_VAR_LOC ();
+	else
+	  *chain->loc = gen_raw_REG (GET_MODE (*chain->loc), reg);
+	df_insn_rescan (chain->insn);
+      }
+
   mode = GET_MODE (*head->first->loc);
   head->regno = reg;
   head->nregs = hard_regno_nregs[reg][mode];