[patch] fix regrename pass to ensure renamings produce valid insns

[patch] fix regrename pass to ensure renamings produce valid insns

[Sandra Loosemore]

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

We ran across problems with the regrename pass introducing invalid insns 
while working on support for new load/store multiple instructions on 
nios2.  We're using an implementation similar to what ARM already does, 
with a match_parallel predicate testing for restrictions on the 
underlying machine instructions that can't be expressed using GCC 
register constraints.  (E.g., the register numbers do not have to be 
sequential, but they do have to be ascending/descending and a 
combination that can be encoded in the machine instruction.)

The attached patch teaches regrename to validate insns affected by each 
register renaming before making the change.  I can see at least two 
other ways to handle this -- earlier, by rejecting renamings that result 
in invalid instructions when it's searching for the best renaming; or 
later, by validating the entire set of renamings as a group instead of 
incrementally for each one -- but doing it all in regname_do_replace 
seems least disruptive and risky in terms of the existing code.

There are a couple of old bugzilla tickets for 
instruction-doesn't-satisfy-its-constraints ICEs in regrename that might 
or might not be related to this.  The symptom we saw was not an ICE, 
just bad code being emitted that resulted in assembler errors.

I've bootstrapped and regression-tested this on x86_64-linux-gnu in 
addition to our testing with the pending nios2 patch set.  OK to commit?

-Sandra


[-- Attachment #2: regrename.log --]
[-- Type: text/x-log, Size: 180 bytes --]

2015-06-17  Chung-Lin Tang <cltang@codesourcery.com>
	    Sandra Loosemore <sandra@codesourcery.com>

	gcc/
	* regrename.c (regrename_do_replace): Re-validate the modified
	insns.

[-- Attachment #3: regrename.patch --]
[-- Type: text/x-patch, Size: 1081 bytes --]

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;
 	  REG_POINTER (*chain->loc) = reg_ptr;
 	}
-
-      df_insn_rescan (chain->insn);
     }
 
+  if (!apply_change_group ())
+    return;
+
   mode = GET_MODE (*head->first->loc);
   head->regno = reg;
   head->nregs = hard_regno_nregs[reg][mode];

Re: [patch] fix regrename pass to ensure renamings produce valid insns

[Richard Sandiford]

Sandra Loosemore <sandra@codesourcery.com> writes:
> 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;
>  	  REG_POINTER (*chain->loc) = reg_ptr;
>  	}

I think it'd be cleaner to do the adjustments on the new register before
passing it to validate_change.

LGTM otherwise for what it's worth.

Thanks,
Richard

Re: [patch] fix regrename pass to ensure renamings produce valid insns

[Eric Botcazou]

> The attached patch teaches regrename to validate insns affected by each
> register renaming before making the change.  I can see at least two
> other ways to handle this -- earlier, by rejecting renamings that result
> in invalid instructions when it's searching for the best renaming; or
> later, by validating the entire set of renamings as a group instead of
> incrementally for each one -- but doing it all in regname_do_replace
> seems least disruptive and risky in terms of the existing code.

OK, but the patch looks incomplete, rename_chains should be adjusted as well, 
i.e. regrename_do_replace should now return a boolean.

-- 
Eric Botcazou

Re: [patch] fix regrename pass to ensure renamings produce valid insns

[Sandra Loosemore]

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

On 06/18/2015 11:32 AM, Eric Botcazou wrote:
>> The attached patch teaches regrename to validate insns affected by each
>> register renaming before making the change.  I can see at least two
>> other ways to handle this -- earlier, by rejecting renamings that result
>> in invalid instructions when it's searching for the best renaming; or
>> later, by validating the entire set of renamings as a group instead of
>> incrementally for each one -- but doing it all in regname_do_replace
>> seems least disruptive and risky in terms of the existing code.
>
> OK, but the patch looks incomplete, rename_chains should be adjusted as well,
> i.e. regrename_do_replace should now return a boolean.

Like this?  I tested this on nios2 and x86_64-linux-gnu, as before, plus 
built for aarch64-linux-gnu and ran the gcc testsuite.

The c6x back end also calls regrename_do_replace.  I am not set up to 
build or test on that target, and Bernd told me off-list that it would 
never fail on that target anyway so I have left that code alone.

-Sandra

[-- Attachment #2: regrename-2.log --]
[-- Type: text/x-log, Size: 433 bytes --]

2015-06-23  Chung-Lin Tang <cltang@codesourcery.com>
	    Sandra Loosemore <sandra@codesourcery.com>

	gcc/
	* regrename.h (regrename_do_replace): Change to return bool.
	* regrename.c (rename_chains): Check return value of
	regname_do_replace.
	(regrename_do_replace): Re-validate the modified insns and
	return bool status.
	* config/aarch64/cortex-a57-fma-steering.c (rename_single_chain):
	Update to match rename_chains changes.

[-- Attachment #3: regrename-2.patch --]
[-- Type: text/x-patch, Size: 3732 bytes --]

Index: gcc/regrename.h
===================================================================
--- gcc/regrename.h	(revision 224700)
+++ gcc/regrename.h	(working copy)
@@ -91,6 +91,6 @@ extern void regrename_analyze (bitmap);
 extern du_head_p regrename_chain_from_id (unsigned int);
 extern int find_rename_reg (du_head_p, enum reg_class, HARD_REG_SET *, int,
 			    bool);
-extern void regrename_do_replace (du_head_p, int);
+extern bool regrename_do_replace (du_head_p, int);
 
 #endif
Index: gcc/regrename.c
===================================================================
--- gcc/regrename.c	(revision 224700)
+++ gcc/regrename.c	(working copy)
@@ -496,12 +496,20 @@ rename_chains (void)
 	  continue;
 	}
 
-      if (dump_file)
-	fprintf (dump_file, ", renamed as %s\n", reg_names[best_new_reg]);
-
-      regrename_do_replace (this_head, best_new_reg);
-      tick[best_new_reg] = ++this_tick;
-      df_set_regs_ever_live (best_new_reg, true);
+      if (regrename_do_replace (this_head, best_new_reg))
+	{
+	  if (dump_file)
+	    fprintf (dump_file, ", renamed as %s\n", reg_names[best_new_reg]);
+	  tick[best_new_reg] = ++this_tick;
+	  df_set_regs_ever_live (best_new_reg, true);
+	}
+      else
+	{
+	  if (dump_file)
+	    fprintf (dump_file, ", renaming as %s failed\n",
+		     reg_names[best_new_reg]);
+	  tick[reg] = ++this_tick;
+	}
     }
 }
 
@@ -927,7 +935,13 @@ regrename_analyze (bitmap bb_mask)
     bb->aux = NULL;
 }
 
-void
+/* Attempt to replace all uses of the register in the chain beginning with
+   HEAD with REG.  Returns true on success and false if the replacement is
+   rejected because the insns would not validate.  The latter can happen
+   e.g. if a match_parallel predicate enforces restrictions on register
+   numbering in its subpatterns.  */
+
+bool
 regrename_do_replace (struct du_head *head, int reg)
 {
   struct du_chain *chain;
@@ -941,22 +955,26 @@ 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;
 	  REG_POINTER (*chain->loc) = reg_ptr;
 	}
-
-      df_insn_rescan (chain->insn);
     }
 
+  if (!apply_change_group ())
+    return false;
+
   mode = GET_MODE (*head->first->loc);
   head->regno = reg;
   head->nregs = hard_regno_nregs[reg][mode];
+  return true;
 }
 
 
Index: gcc/config/aarch64/cortex-a57-fma-steering.c
===================================================================
--- gcc/config/aarch64/cortex-a57-fma-steering.c	(revision 224700)
+++ gcc/config/aarch64/cortex-a57-fma-steering.c	(working copy)
@@ -288,11 +288,19 @@ rename_single_chain (du_head_p head, HAR
       return false;
     }
 
-  if (dump_file)
-    fprintf (dump_file, ", renamed as %s\n", reg_names[best_new_reg]);
-
-  regrename_do_replace (head, best_new_reg);
-  df_set_regs_ever_live (best_new_reg, true);
+  if (regrename_do_replace (head, best_new_reg))
+    {
+      if (dump_file)
+	fprintf (dump_file, ", renamed as %s\n", reg_names[best_new_reg]);
+      df_set_regs_ever_live (best_new_reg, true);
+    }
+  else
+    {
+      if (dump_file)
+	fprintf (dump_file, ", renaming as %s failed\n",
+		 reg_names[best_new_reg]);
+      return false;
+    }
   return true;
 }
 

Re: [patch] fix regrename pass to ensure renamings produce valid insns

[Ramana Radhakrishnan]

On 24/06/15 02:00, Sandra Loosemore wrote:
> On 06/18/2015 11:32 AM, Eric Botcazou wrote:
>>> The attached patch teaches regrename to validate insns affected by each
>>> register renaming before making the change.  I can see at least two
>>> other ways to handle this -- earlier, by rejecting renamings that result
>>> in invalid instructions when it's searching for the best renaming; or
>>> later, by validating the entire set of renamings as a group instead of
>>> incrementally for each one -- but doing it all in regname_do_replace
>>> seems least disruptive and risky in terms of the existing code.
>>
>> OK, but the patch looks incomplete, rename_chains should be adjusted
>> as well,
>> i.e. regrename_do_replace should now return a boolean.
>
> Like this?  I tested this on nios2 and x86_64-linux-gnu, as before, plus
> built for aarch64-linux-gnu and ran the gcc testsuite.

Hopefully that was built with --with-cpu=cortex-a57 to enable the 
renaming pass ?

Ramana

>
> The c6x back end also calls regrename_do_replace.  I am not set up to
> build or test on that target, and Bernd told me off-list that it would
> never fail on that target anyway so I have left that code alone.
>
> -Sandra

Re: [patch] fix regrename pass to ensure renamings produce valid insns

[Sandra Loosemore]

On 06/24/2015 01:58 AM, Ramana Radhakrishnan wrote:
>
>
> On 24/06/15 02:00, Sandra Loosemore wrote:
>> On 06/18/2015 11:32 AM, Eric Botcazou wrote:
>>>> The attached patch teaches regrename to validate insns affected by each
>>>> register renaming before making the change.  I can see at least two
>>>> other ways to handle this -- earlier, by rejecting renamings that
>>>> result
>>>> in invalid instructions when it's searching for the best renaming; or
>>>> later, by validating the entire set of renamings as a group instead of
>>>> incrementally for each one -- but doing it all in regname_do_replace
>>>> seems least disruptive and risky in terms of the existing code.
>>>
>>> OK, but the patch looks incomplete, rename_chains should be adjusted
>>> as well,
>>> i.e. regrename_do_replace should now return a boolean.
>>
>> Like this?  I tested this on nios2 and x86_64-linux-gnu, as before, plus
>> built for aarch64-linux-gnu and ran the gcc testsuite.
>
> Hopefully that was built with --with-cpu=cortex-a57 to enable the
> renaming pass ?

No, sorry.  I was assuming there were compile-only unit tests for this 
pass that automatically add the right options to enable it.  I don't 
know that I can actually run cortex-a57 code (I was struggling with a 
flaky test harness as it was).

-Sandra

Re: [patch] fix regrename pass to ensure renamings produce valid insns

[Eric Botcazou]

> Like this?  I tested this on nios2 and x86_64-linux-gnu, as before, plus
> built for aarch64-linux-gnu and ran the gcc testsuite.

Yes, the patch is OK, modulo...

> The c6x back end also calls regrename_do_replace.  I am not set up to
> build or test on that target, and Bernd told me off-list that it would
> never fail on that target anyway so I have left that code alone.

... Bernd has obviously the final say here, but it would be better to add an 
assertion that it indeed did not fail (just build the cc1 as a sanity check).

Thanks for adding the missing head comment to regrename_do_replace.

-- 
Eric Botcazou

Re: [patch] fix regrename pass to ensure renamings produce valid insns

[Eric Botcazou]

> Yes, the patch is OK, modulo...

But you also need the approval of an ARM maintainer.

-- 
Eric Botcazou

Re: [patch] fix regrename pass to ensure renamings produce valid insns

[Jeff Law]

On 06/23/2015 07:00 PM, Sandra Loosemore wrote:
> On 06/18/2015 11:32 AM, Eric Botcazou wrote:
>>> The attached patch teaches regrename to validate insns affected by each
>>> register renaming before making the change.  I can see at least two
>>> other ways to handle this -- earlier, by rejecting renamings that result
>>> in invalid instructions when it's searching for the best renaming; or
>>> later, by validating the entire set of renamings as a group instead of
>>> incrementally for each one -- but doing it all in regname_do_replace
>>> seems least disruptive and risky in terms of the existing code.
>>
>> OK, but the patch looks incomplete, rename_chains should be adjusted
>> as well,
>> i.e. regrename_do_replace should now return a boolean.
>
> Like this?  I tested this on nios2 and x86_64-linux-gnu, as before, plus
> built for aarch64-linux-gnu and ran the gcc testsuite.
>
> The c6x back end also calls regrename_do_replace.  I am not set up to
> build or test on that target, and Bernd told me off-list that it would
> never fail on that target anyway so I have left that code alone.
>
> -Sandra
>
> regrename-2.log
>
>
> 2015-06-23  Chung-Lin Tang<cltang@codesourcery.com>
> 	    Sandra Loosemore<sandra@codesourcery.com>
>
> 	gcc/
> 	* regrename.h (regrename_do_replace): Change to return bool.
> 	* regrename.c (rename_chains): Check return value of
> 	regname_do_replace.
> 	(regrename_do_replace): Re-validate the modified insns and
> 	return bool status.
> 	* config/aarch64/cortex-a57-fma-steering.c (rename_single_chain):
> 	Update to match rename_chains changes.
As Eric mentioned, please put an assert to verify that the call from the 
c6x backend never fails.

The regrename and ARM bits are fine.

Do you have a testcase that you can add to the suite?  If so it'd be 
appreciated if you could include that too.

Approved with the c6x assert if a testcase isn't available or 
exceedingly difficult to produce.

jeff

Re: [patch] fix regrename pass to ensure renamings produce valid insns

[Bernd Schmidt]

On 06/25/2015 05:46 AM, Jeff Law wrote:
> As Eric mentioned, please put an assert to verify that the call from the
> c6x backend never fails.

I'd be happiest if we had an assert on almost all targets. regrename has 
been designed in such a way that the replacements can't fail, if the 
constraints on the insns are correct. If there are ports which have 
borderline insns where that doesn't hold maybe we ought to have a target 
hook that says so.


Bernd

Re: [patch] fix regrename pass to ensure renamings produce valid insns

[Eric Botcazou]

> I'd be happiest if we had an assert on almost all targets. regrename has
> been designed in such a way that the replacements can't fail, if the
> constraints on the insns are correct. If there are ports which have
> borderline insns where that doesn't hold maybe we ought to have a target
> hook that says so.

ARM both has "borderline" insns (in fact insns with match_parallel are enough) 
and benefits from the regrename pass so this doesn't seem to be really doable.

-- 
Eric Botcazou

Re: [patch] fix regrename pass to ensure renamings produce valid insns

[Bernd Schmidt]

On 06/25/2015 03:53 PM, Eric Botcazou wrote:
>> I'd be happiest if we had an assert on almost all targets. regrename has
>> been designed in such a way that the replacements can't fail, if the
>> constraints on the insns are correct. If there are ports which have
>> borderline insns where that doesn't hold maybe we ought to have a target
>> hook that says so.
>
> ARM both has "borderline" insns (in fact insns with match_parallel are enough)
> and benefits from the regrename pass so this doesn't seem to be really doable.

All I'm saying it would be nice to have a port say "validate regrename 
results", and use the assert only if the port doesn't request it. On 
most ports (excluding arm and nios2 by the sound of it) we'd still have 
an assert verifying that the regrename implementation is sound wrt doing 
the right thing with insn constraints.


Bernd

Re: [patch] fix regrename pass to ensure renamings produce valid insns

[Sandra Loosemore]

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

On 06/24/2015 09:46 PM, Jeff Law wrote:
> On 06/23/2015 07:00 PM, Sandra Loosemore wrote:
>> On 06/18/2015 11:32 AM, Eric Botcazou wrote:
>>>> The attached patch teaches regrename to validate insns affected by each
>>>> register renaming before making the change.  I can see at least two
>>>> other ways to handle this -- earlier, by rejecting renamings that
>>>> result
>>>> in invalid instructions when it's searching for the best renaming; or
>>>> later, by validating the entire set of renamings as a group instead of
>>>> incrementally for each one -- but doing it all in regname_do_replace
>>>> seems least disruptive and risky in terms of the existing code.
>>>
>>> OK, but the patch looks incomplete, rename_chains should be adjusted
>>> as well,
>>> i.e. regrename_do_replace should now return a boolean.
>>
>> Like this?  I tested this on nios2 and x86_64-linux-gnu, as before, plus
>> built for aarch64-linux-gnu and ran the gcc testsuite.
>>
>> The c6x back end also calls regrename_do_replace.  I am not set up to
>> build or test on that target, and Bernd told me off-list that it would
>> never fail on that target anyway so I have left that code alone.
>>
>> -Sandra
>>
>> regrename-2.log
>>
>>
>> 2015-06-23  Chung-Lin Tang<cltang@codesourcery.com>
>>         Sandra Loosemore<sandra@codesourcery.com>
>>
>>     gcc/
>>     * regrename.h (regrename_do_replace): Change to return bool.
>>     * regrename.c (rename_chains): Check return value of
>>     regname_do_replace.
>>     (regrename_do_replace): Re-validate the modified insns and
>>     return bool status.
>>     * config/aarch64/cortex-a57-fma-steering.c (rename_single_chain):
>>     Update to match rename_chains changes.
> As Eric mentioned, please put an assert to verify that the call from the
> c6x backend never fails.
>
> The regrename and ARM bits are fine.
>
> Do you have a testcase that you can add to the suite?  If so it'd be
> appreciated if you could include that too.
>
> Approved with the c6x assert if a testcase isn't available or
> exceedingly difficult to produce.

Thanks.  I've committed the attached version.

Re the testcase, this fixed 16 FAILs on existing tests in the gcc 
testsuite with the forthcoming nios2 load/store multiple instruction 
support, all assembler errors due to the bad instructions being 
generated.  There's nothing I can do on nios2 for a testcase until I get 
those patches committed (I'm still trying to re-test and tidy them up 
for submission), plus I think the failures are rather fragile -- 
depending on the register allocator choosing an initial register 
numbering that allows peephole optimizers to trigger, etc.  But, I will 
revisit this later and see what I can do.

-Sandra


[-- Attachment #2: regrename-3.log --]
[-- Type: text/x-log, Size: 524 bytes --]

2015-06-28  Chung-Lin Tang <cltang@codesourcery.com>
	    Sandra Loosemore <sandra@codesourcery.com>

	gcc/
	* regrename.h (regrename_do_replace): Change to return bool.
	* regrename.c (rename_chains): Check return value of
	regname_do_replace.
	(regrename_do_replace): Re-validate the modified insns and
	return bool status.
	* config/aarch64/cortex-a57-fma-steering.c (rename_single_chain):
	Update to match rename_chains changes.
	* config/c6x/c6x.c (try_rename_operands): Assert that
	regrename_do_replace returns true.

[-- Attachment #3: regrename-3.patch --]
[-- Type: text/x-patch, Size: 4639 bytes --]

Index: gcc/regrename.c
===================================================================
--- gcc/regrename.c	(revision 225104)
+++ gcc/regrename.c	(working copy)
@@ -496,12 +496,20 @@ rename_chains (void)
 	  continue;
 	}
 
-      if (dump_file)
-	fprintf (dump_file, ", renamed as %s\n", reg_names[best_new_reg]);
-
-      regrename_do_replace (this_head, best_new_reg);
-      tick[best_new_reg] = ++this_tick;
-      df_set_regs_ever_live (best_new_reg, true);
+      if (regrename_do_replace (this_head, best_new_reg))
+	{
+	  if (dump_file)
+	    fprintf (dump_file, ", renamed as %s\n", reg_names[best_new_reg]);
+	  tick[best_new_reg] = ++this_tick;
+	  df_set_regs_ever_live (best_new_reg, true);
+	}
+      else
+	{
+	  if (dump_file)
+	    fprintf (dump_file, ", renaming as %s failed\n",
+		     reg_names[best_new_reg]);
+	  tick[reg] = ++this_tick;
+	}
     }
 }
 
@@ -927,7 +935,13 @@ regrename_analyze (bitmap bb_mask)
     bb->aux = NULL;
 }
 
-void
+/* Attempt to replace all uses of the register in the chain beginning with
+   HEAD with REG.  Returns true on success and false if the replacement is
+   rejected because the insns would not validate.  The latter can happen
+   e.g. if a match_parallel predicate enforces restrictions on register
+   numbering in its subpatterns.  */
+
+bool
 regrename_do_replace (struct du_head *head, int reg)
 {
   struct du_chain *chain;
@@ -941,22 +955,26 @@ 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;
 	  REG_POINTER (*chain->loc) = reg_ptr;
 	}
-
-      df_insn_rescan (chain->insn);
     }
 
+  if (!apply_change_group ())
+    return false;
+
   mode = GET_MODE (*head->first->loc);
   head->regno = reg;
   head->nregs = hard_regno_nregs[reg][mode];
+  return true;
 }
 
 
Index: gcc/regrename.h
===================================================================
--- gcc/regrename.h	(revision 225104)
+++ gcc/regrename.h	(working copy)
@@ -91,6 +91,6 @@ extern void regrename_analyze (bitmap);
 extern du_head_p regrename_chain_from_id (unsigned int);
 extern int find_rename_reg (du_head_p, enum reg_class, HARD_REG_SET *, int,
 			    bool);
-extern void regrename_do_replace (du_head_p, int);
+extern bool regrename_do_replace (du_head_p, int);
 
 #endif
Index: gcc/config/aarch64/cortex-a57-fma-steering.c
===================================================================
--- gcc/config/aarch64/cortex-a57-fma-steering.c	(revision 225104)
+++ gcc/config/aarch64/cortex-a57-fma-steering.c	(working copy)
@@ -289,11 +289,19 @@ rename_single_chain (du_head_p head, HAR
       return false;
     }
 
-  if (dump_file)
-    fprintf (dump_file, ", renamed as %s\n", reg_names[best_new_reg]);
-
-  regrename_do_replace (head, best_new_reg);
-  df_set_regs_ever_live (best_new_reg, true);
+  if (regrename_do_replace (head, best_new_reg))
+    {
+      if (dump_file)
+	fprintf (dump_file, ", renamed as %s\n", reg_names[best_new_reg]);
+      df_set_regs_ever_live (best_new_reg, true);
+    }
+  else
+    {
+      if (dump_file)
+	fprintf (dump_file, ", renaming as %s failed\n",
+		 reg_names[best_new_reg]);
+      return false;
+    }
   return true;
 }
 
Index: gcc/config/c6x/c6x.c
===================================================================
--- gcc/config/c6x/c6x.c	(revision 225104)
+++ gcc/config/c6x/c6x.c	(working copy)
@@ -3516,7 +3516,7 @@ try_rename_operands (rtx_insn *head, rtx
   best_reg =
     find_rename_reg (this_head, super_class, &unavailable, old_reg, true);
 
-  regrename_do_replace (this_head, best_reg);
+  gcc_assert (regrename_do_replace (this_head, best_reg));
 
   count_unit_reqs (new_reqs, head, PREV_INSN (tail));
   merge_unit_reqs (new_reqs);
@@ -3529,7 +3529,7 @@ try_rename_operands (rtx_insn *head, rtx
 	       unit_req_imbalance (reqs), unit_req_imbalance (new_reqs));
     }
   if (unit_req_imbalance (new_reqs) > unit_req_imbalance (reqs))
-    regrename_do_replace (this_head, old_reg);
+    gcc_assert (regrename_do_replace (this_head, old_reg));
   else
     memcpy (reqs, new_reqs, sizeof (unit_req_table));
 

Re: [patch] fix regrename pass to ensure renamings produce valid insns

[Kito Cheng]

Hi all:

This patch seem will broken when disable assert checking for c6x....

Index: gcc/config/c6x/c6x.c
===================================================================
--- gcc/config/c6x/c6x.c (revision 225104)
+++ gcc/config/c6x/c6x.c (working copy)
@@ -3516,7 +3516,7 @@ try_rename_operands (rtx_insn *head, rtx
   best_reg =
     find_rename_reg (this_head, super_class, &unavailable, old_reg, true);

-  regrename_do_replace (this_head, best_reg);
+  gcc_assert (regrename_do_replace (this_head, best_reg));

   count_unit_reqs (new_reqs, head, PREV_INSN (tail));
   merge_unit_reqs (new_reqs);
@@ -3529,7 +3529,7 @@ try_rename_operands (rtx_insn *head, rtx
        unit_req_imbalance (reqs), unit_req_imbalance (new_reqs));
     }
   if (unit_req_imbalance (new_reqs) > unit_req_imbalance (reqs))
-    regrename_do_replace (this_head, old_reg);
+    gcc_assert (regrename_do_replace (this_head, old_reg));
   else
     memcpy (reqs, new_reqs, sizeof (unit_req_table));


On Mon, Jun 29, 2015 at 5:08 AM, Sandra Loosemore
<sandra@codesourcery.com> wrote:
> On 06/24/2015 09:46 PM, Jeff Law wrote:
>>
>> On 06/23/2015 07:00 PM, Sandra Loosemore wrote:
>>>
>>> On 06/18/2015 11:32 AM, Eric Botcazou wrote:
>>>>>
>>>>> The attached patch teaches regrename to validate insns affected by each
>>>>> register renaming before making the change.  I can see at least two
>>>>> other ways to handle this -- earlier, by rejecting renamings that
>>>>> result
>>>>> in invalid instructions when it's searching for the best renaming; or
>>>>> later, by validating the entire set of renamings as a group instead of
>>>>> incrementally for each one -- but doing it all in regname_do_replace
>>>>> seems least disruptive and risky in terms of the existing code.
>>>>
>>>>
>>>> OK, but the patch looks incomplete, rename_chains should be adjusted
>>>> as well,
>>>> i.e. regrename_do_replace should now return a boolean.
>>>
>>>
>>> Like this?  I tested this on nios2 and x86_64-linux-gnu, as before, plus
>>> built for aarch64-linux-gnu and ran the gcc testsuite.
>>>
>>> The c6x back end also calls regrename_do_replace.  I am not set up to
>>> build or test on that target, and Bernd told me off-list that it would
>>> never fail on that target anyway so I have left that code alone.
>>>
>>> -Sandra
>>>
>>> regrename-2.log
>>>
>>>
>>> 2015-06-23  Chung-Lin Tang<cltang@codesourcery.com>
>>>         Sandra Loosemore<sandra@codesourcery.com>
>>>
>>>     gcc/
>>>     * regrename.h (regrename_do_replace): Change to return bool.
>>>     * regrename.c (rename_chains): Check return value of
>>>     regname_do_replace.
>>>     (regrename_do_replace): Re-validate the modified insns and
>>>     return bool status.
>>>     * config/aarch64/cortex-a57-fma-steering.c (rename_single_chain):
>>>     Update to match rename_chains changes.
>>
>> As Eric mentioned, please put an assert to verify that the call from the
>> c6x backend never fails.
>>
>> The regrename and ARM bits are fine.
>>
>> Do you have a testcase that you can add to the suite?  If so it'd be
>> appreciated if you could include that too.
>>
>> Approved with the c6x assert if a testcase isn't available or
>> exceedingly difficult to produce.
>
>
> Thanks.  I've committed the attached version.
>
> Re the testcase, this fixed 16 FAILs on existing tests in the gcc testsuite
> with the forthcoming nios2 load/store multiple instruction support, all
> assembler errors due to the bad instructions being generated.  There's
> nothing I can do on nios2 for a testcase until I get those patches committed
> (I'm still trying to re-test and tidy them up for submission), plus I think
> the failures are rather fragile -- depending on the register allocator
> choosing an initial register numbering that allows peephole optimizers to
> trigger, etc.  But, I will revisit this later and see what I can do.
>
> -Sandra
>

Re: [patch] fix regrename pass to ensure renamings produce valid insns

[Sandra Loosemore]

On 06/29/2015 09:07 PM, Kito Cheng wrote:
> Hi all:
>
> This patch seem will broken when disable assert checking for c6x....
>
> Index: gcc/config/c6x/c6x.c
> ===================================================================
> --- gcc/config/c6x/c6x.c (revision 225104)
> +++ gcc/config/c6x/c6x.c (working copy)
> @@ -3516,7 +3516,7 @@ try_rename_operands (rtx_insn *head, rtx
>     best_reg =
>       find_rename_reg (this_head, super_class, &unavailable, old_reg, true);
>
> -  regrename_do_replace (this_head, best_reg);
> +  gcc_assert (regrename_do_replace (this_head, best_reg));
>
>     count_unit_reqs (new_reqs, head, PREV_INSN (tail));
>     merge_unit_reqs (new_reqs);
> @@ -3529,7 +3529,7 @@ try_rename_operands (rtx_insn *head, rtx
>          unit_req_imbalance (reqs), unit_req_imbalance (new_reqs));
>       }
>     if (unit_req_imbalance (new_reqs) > unit_req_imbalance (reqs))
> -    regrename_do_replace (this_head, old_reg);
> +    gcc_assert (regrename_do_replace (this_head, old_reg));
>     else
>       memcpy (reqs, new_reqs, sizeof (unit_req_table));
>

I'm sorry; do you have a suggestion for a fix?  I thought this was the 
change I was asked to make, and as I noted previously, I'm not set up to 
test (or even build) for this target.

-Sandra the obviously confused :-(

Re: [patch] fix regrename pass to ensure renamings produce valid insns

[Chung-Lin Tang]

On 2015/6/30 12:22 PM, Sandra Loosemore wrote:
> On 06/29/2015 09:07 PM, Kito Cheng wrote:
>> Hi all:
>>
>> This patch seem will broken when disable assert checking for c6x....
>>
>> Index: gcc/config/c6x/c6x.c
>> ===================================================================
>> --- gcc/config/c6x/c6x.c (revision 225104)
>> +++ gcc/config/c6x/c6x.c (working copy)
>> @@ -3516,7 +3516,7 @@ try_rename_operands (rtx_insn *head, rtx
>>     best_reg =
>>       find_rename_reg (this_head, super_class, &unavailable, old_reg, true);
>>
>> -  regrename_do_replace (this_head, best_reg);
>> +  gcc_assert (regrename_do_replace (this_head, best_reg));
>>
>>     count_unit_reqs (new_reqs, head, PREV_INSN (tail));
>>     merge_unit_reqs (new_reqs);
>> @@ -3529,7 +3529,7 @@ try_rename_operands (rtx_insn *head, rtx
>>          unit_req_imbalance (reqs), unit_req_imbalance (new_reqs));
>>       }
>>     if (unit_req_imbalance (new_reqs) > unit_req_imbalance (reqs))
>> -    regrename_do_replace (this_head, old_reg);
>> +    gcc_assert (regrename_do_replace (this_head, old_reg));
>>     else
>>       memcpy (reqs, new_reqs, sizeof (unit_req_table));
>>
> 
> I'm sorry; do you have a suggestion for a fix?  I thought this was the change I was asked to make, and as I noted previously, I'm not set up to test (or even build) for this target.
> 
> -Sandra the obviously confused :-(

You probably have to separate out the regrename_do_replace() bool result into a variable,
placing the whole call into the gcc_assert() might make it disappear when assertions are turned off.

Chung-Lin

Re: [patch] fix regrename pass to ensure renamings produce valid insns

[Chung-Lin Tang]

On 2015/6/30 下午 01:13, Chung-Lin Tang wrote:
> On 2015/6/30 12:22 PM, Sandra Loosemore wrote:
>> On 06/29/2015 09:07 PM, Kito Cheng wrote:
>>> Hi all:
>>>
>>> This patch seem will broken when disable assert checking for c6x....
>>>
>>> Index: gcc/config/c6x/c6x.c
>>> ===================================================================
>>> --- gcc/config/c6x/c6x.c (revision 225104)
>>> +++ gcc/config/c6x/c6x.c (working copy)
>>> @@ -3516,7 +3516,7 @@ try_rename_operands (rtx_insn *head, rtx
>>>     best_reg =
>>>       find_rename_reg (this_head, super_class, &unavailable, old_reg, true);
>>>
>>> -  regrename_do_replace (this_head, best_reg);
>>> +  gcc_assert (regrename_do_replace (this_head, best_reg));
>>>
>>>     count_unit_reqs (new_reqs, head, PREV_INSN (tail));
>>>     merge_unit_reqs (new_reqs);
>>> @@ -3529,7 +3529,7 @@ try_rename_operands (rtx_insn *head, rtx
>>>          unit_req_imbalance (reqs), unit_req_imbalance (new_reqs));
>>>       }
>>>     if (unit_req_imbalance (new_reqs) > unit_req_imbalance (reqs))
>>> -    regrename_do_replace (this_head, old_reg);
>>> +    gcc_assert (regrename_do_replace (this_head, old_reg));
>>>     else
>>>       memcpy (reqs, new_reqs, sizeof (unit_req_table));
>>>
>>
>> I'm sorry; do you have a suggestion for a fix?  I thought this was the change I was asked to make, and as I noted previously, I'm not set up to test (or even build) for this target.
>>
>> -Sandra the obviously confused :-(
> 
> You probably have to separate out the regrename_do_replace() bool result into a variable,
> placing the whole call into the gcc_assert() might make it disappear when assertions are turned off.

I notice the way gcc_assert() is defined in system.h now, the test won't disappear even when runtime checks
are disabled, though you might still adjust it to avoid any programmer confusion.

Chung-Lin

Re: [patch] fix regrename pass to ensure renamings produce valid insns

[Eric Botcazou]

> I notice the way gcc_assert() is defined in system.h now, the test won't
> disappear even when runtime checks are disabled, though you might still
> adjust it to avoid any programmer confusion.

It will disappear at run time, see the definition:

/* Include EXPR, so that unused variable warnings do not occur.  */
#define gcc_assert(EXPR) ((void)(0 && (EXPR)))

so you really need to use a separate variable.

-- 
Eric Botcazou

Re: [patch] fix regrename pass to ensure renamings produce valid insns

[Chung-Lin Tang]

On 2015/6/30 05:06 PM, Eric Botcazou wrote:
>> I notice the way gcc_assert() is defined in system.h now, the test won't
>> disappear even when runtime checks are disabled, though you might still
>> adjust it to avoid any programmer confusion.
> 
> It will disappear at run time, see the definition:
> 
> /* Include EXPR, so that unused variable warnings do not occur.  */
> #define gcc_assert(EXPR) ((void)(0 && (EXPR)))
> 
> so you really need to use a separate variable.
> 

I was referring to this one:
#if ENABLE_ASSERT_CHECKING
...	
#elif (GCC_VERSION >= 4005)
#define gcc_assert(EXPR)                                                \
  ((void)(__builtin_expect (!(EXPR), 0) ? __builtin_unreachable (), 0 : 0))
#else
...

But yeah, I guess older GCCs could be used to build a toolchain,
so a separate variable should be used.

Chung-Lin

Re: [patch] fix regrename pass to ensure renamings produce valid insns

[Sandra Loosemore]

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

On 06/30/2015 03:06 AM, Eric Botcazou wrote:
>> I notice the way gcc_assert() is defined in system.h now, the test won't
>> disappear even when runtime checks are disabled, though you might still
>> adjust it to avoid any programmer confusion.
>
> It will disappear at run time, see the definition:
>
> /* Include EXPR, so that unused variable warnings do not occur.  */
> #define gcc_assert(EXPR) ((void)(0 && (EXPR)))
>
> so you really need to use a separate variable.

Oh, yuck -- it never even occurred to me that gcc_assert could be 
disabled.  I'll bet there are other bugs in GCC due to this very same 
problem of depending on its argument being executed for side-effect. 
(E.g. take a look at add_stmt_to_eh_lp_fn in tree-eh.c.)  Seems like 
lousy design to me especially since proper usage doesn't seem to be 
documented anywhere.

Anyway, I think the attached patch is what's required to fix the 
instance that's my fault.  OK?  Bernd, if this needs testing, can you help?

-Sandra

[-- Attachment #2: regrename-4.log --]
[-- Type: text/x-log, Size: 175 bytes --]

2015-06-30  Sandra Loosemore <sandra@codesourcery.com>

	gcc/
	* config/c6x/c6x.c (try_rename_operands): Do not depend on
	gcc_assert evaluating its argument for side-effect.

[-- Attachment #3: regrename-4.patch --]
[-- Type: text/x-patch, Size: 1209 bytes --]

Index: gcc/config/c6x/c6x.c
===================================================================
--- gcc/config/c6x/c6x.c	(revision 225202)
+++ gcc/config/c6x/c6x.c	(working copy)
@@ -3450,6 +3450,7 @@ try_rename_operands (rtx_insn *head, rtx
   int best_reg, old_reg;
   vec<du_head_p> involved_chains = vNULL;
   unit_req_table new_reqs;
+  bool ok;
 
   for (i = 0, tmp_mask = op_mask; tmp_mask; i++)
     {
@@ -3516,7 +3517,8 @@ try_rename_operands (rtx_insn *head, rtx
   best_reg =
     find_rename_reg (this_head, super_class, &unavailable, old_reg, true);
 
-  gcc_assert (regrename_do_replace (this_head, best_reg));
+  ok = regrename_do_replace (this_head, best_reg);
+  gcc_assert (ok);
 
   count_unit_reqs (new_reqs, head, PREV_INSN (tail));
   merge_unit_reqs (new_reqs);
@@ -3529,7 +3531,10 @@ try_rename_operands (rtx_insn *head, rtx
 	       unit_req_imbalance (reqs), unit_req_imbalance (new_reqs));
     }
   if (unit_req_imbalance (new_reqs) > unit_req_imbalance (reqs))
-    gcc_assert (regrename_do_replace (this_head, old_reg));
+    {
+      ok = regrename_do_replace (this_head, old_reg);
+      gcc_assert (ok);
+    }
   else
     memcpy (reqs, new_reqs, sizeof (unit_req_table));
 

Re: [patch] fix regrename pass to ensure renamings produce valid insns

[Eric Botcazou]

> Oh, yuck -- it never even occurred to me that gcc_assert could be
> disabled.  I'll bet there are other bugs in GCC due to this very same
> problem of depending on its argument being executed for side-effect.

Very likely so...

> (E.g. take a look at add_stmt_to_eh_lp_fn in tree-eh.c.)  Seems like
> lousy design to me especially since proper usage doesn't seem to be
> documented anywhere.

... but not this one though.

> Anyway, I think the attached patch is what's required to fix the
> instance that's my fault.  OK?

Yes, it's obviously OK.

-- 
Eric Botcazou

Re: [patch] fix regrename pass to ensure renamings produce valid insns

[Jeff Law]

On 06/28/2015 03:08 PM, Sandra Loosemore wrote:
>
> Re the testcase, this fixed 16 FAILs on existing tests in the gcc
> testsuite with the forthcoming nios2 load/store multiple instruction
> support, all assembler errors due to the bad instructions being
> generated.  There's nothing I can do on nios2 for a testcase until I get
> those patches committed (I'm still trying to re-test and tidy them up
> for submission), plus I think the failures are rather fragile --
> depending on the register allocator choosing an initial register
> numbering that allows peephole optimizers to trigger, etc.  But, I will
> revisit this later and see what I can do.
OK.  As long as there's going to be some tests, that works for me.

jeff

Re: [patch] fix regrename pass to ensure renamings produce valid insns

[Bernd Schmidt]

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

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


[-- Attachment #2: rr-dinsn.diff --]
[-- Type: text/x-patch, Size: 1313 bytes --]

	* 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];

Re: [patch] fix regrename pass to ensure renamings produce valid insns

[Jeff Law]

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

Re: [patch] fix regrename pass to ensure renamings produce valid insns

[Bernd Schmidt]

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