diff mbox

Fix PR49154-related SEGV in IRA. And ping.

Message ID alpine.BSF.2.00.1106082155240.55402@dair.pair.com
State New
Headers show

Commit Message

Hans-Peter Nilsson June 9, 2011, 2:27 a.m. UTC
First, a ping for
<http://gcc.gnu.org/ml/gcc-patches/2011-06/msg00136.html>;
updated regclass documentation.  Ok?

Second, I updated the CRIS port to fit the proposed
documentation update (adding a class as the patch you sent, but
more complete), with regtest results clean for revisions before
the revision where build started failing.  But, after that
revision, I get a SEGV; details added to
<http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49154>.  At first
I thought I messed up the regclass description but it appears to
be just an obvious miss, fixed below.  If it isn't, I think
setup_regno_cost_classes_by_mode should have a comment as to who
else should set up regno_cost_classes[regno] and that "who else"
needs to be fixed to do the setup.  Odd that this bug didn't
trig before or for any of the targets on which you tested.
Maybe that makes cris-elf qualify for the set of targets you
test IRA changes on?

By the way, can we do a s/_aclass/_class/ in applicable files,
for example ira-costs.c?  Someone appears to have done a botched
word-replace of class to aclass; besides the identifier "class"
it changed occurrences within identifiers too (at least those
after a _) so we have e.g. cost_classes_aclass_cache and
setup_regno_cost_classes_by_aclass vs. cost_classes_mode_cache
and setup_regno_cost_classes_by_mode.

Tested on cris-elf; together with the mentioned update it
restores build with results consistent with those before the
breakage.

Ok to commit?

gcc:
	PR rtl-optimization/49154
	* ira-costs.c (setup_regno_cost_classes_by_mode): If there
	already is a matching slot in the hashtable, assign it to
	classes_ptr.


brgds, H-P

Comments

Vladimir Makarov June 9, 2011, 4:19 p.m. UTC | #1
On 06/08/2011 10:27 PM, Hans-Peter Nilsson wrote:
> First, a ping for
> <http://gcc.gnu.org/ml/gcc-patches/2011-06/msg00136.html>;
> updated regclass documentation.  Ok?
>

Sorry for the delay.  I actually thought about a better formulation for 
some time and forgot about this because I had to work on other urgent 
things.

I'd not say that is the best description of the requirement but I can 
not formulate the better one which is not too complicated.  So it is 
better to have something close than nothing.  It is ok for me.

> Second, I updated the CRIS port to fit the proposed
> documentation update (adding a class as the patch you sent, but
> more complete), with regtest results clean for revisions before
> the revision where build started failing.  But, after that
> revision, I get a SEGV; details added to
> <http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49154>.  At first
> I thought I messed up the regclass description but it appears to
> be just an obvious miss, fixed below.  If it isn't, I think
> setup_regno_cost_classes_by_mode should have a comment as to who
> else should set up regno_cost_classes[regno] and that "who else"
> needs to be fixed to do the setup.  Odd that this bug didn't
> trig before or for any of the targets on which you tested.
> Maybe that makes cris-elf qualify for the set of targets you
> test IRA changes on?
>
> By the way, can we do a s/_aclass/_class/ in applicable files,
> for example ira-costs.c?  Someone appears to have done a botched
> word-replace of class to aclass; besides the identifier "class"
> it changed occurrences within identifiers too (at least those
> after a _) so we have e.g. cost_classes_aclass_cache and
> setup_regno_cost_classes_by_aclass vs. cost_classes_mode_cache
> and setup_regno_cost_classes_by_mode.
>
> Tested on cris-elf; together with the mentioned update it
> restores build with results consistent with those before the
> breakage.
>
> Ok to commit?
>

Yes.  Thanks for fixing this.

> gcc:
> 	PR rtl-optimization/49154
> 	* ira-costs.c (setup_regno_cost_classes_by_mode): If there
> 	already is a matching slot in the hashtable, assign it to
> 	classes_ptr.
>
> diff --git gcc/ira-costs.c gcc/ira-costs.c
> index f517386..a22bb15 100644
> --- gcc/ira-costs.c
> +++ gcc/ira-costs.c
> @@ -299,6 +299,8 @@ setup_regno_cost_classes_by_mode (int regno, enum machine_mode mode)
>   	  classes_ptr = setup_cost_classes (&classes);
>   	  *slot = classes_ptr;
>   	}
> +      else
> +	classes_ptr = *slot;
>         cost_classes_mode_cache[mode] = (cost_classes_t) *slot;
>       }
>     regno_cost_classes[regno] = classes_ptr;
>
> brgds, H-P
H.J. Lu June 10, 2011, 12:50 a.m. UTC | #2
On Wed, Jun 8, 2011 at 7:27 PM, Hans-Peter Nilsson <hp@bitrange.com> wrote:
> First, a ping for
> <http://gcc.gnu.org/ml/gcc-patches/2011-06/msg00136.html>;
> updated regclass documentation.  Ok?
>
> Second, I updated the CRIS port to fit the proposed
> documentation update (adding a class as the patch you sent, but
> more complete), with regtest results clean for revisions before
> the revision where build started failing.  But, after that
> revision, I get a SEGV; details added to
> <http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49154>.  At first
> I thought I messed up the regclass description but it appears to
> be just an obvious miss, fixed below.  If it isn't, I think
> setup_regno_cost_classes_by_mode should have a comment as to who
> else should set up regno_cost_classes[regno] and that "who else"
> needs to be fixed to do the setup.  Odd that this bug didn't
> trig before or for any of the targets on which you tested.
> Maybe that makes cris-elf qualify for the set of targets you
> test IRA changes on?
>
> By the way, can we do a s/_aclass/_class/ in applicable files,
> for example ira-costs.c?  Someone appears to have done a botched
> word-replace of class to aclass; besides the identifier "class"
> it changed occurrences within identifiers too (at least those
> after a _) so we have e.g. cost_classes_aclass_cache and
> setup_regno_cost_classes_by_aclass vs. cost_classes_mode_cache
> and setup_regno_cost_classes_by_mode.
>
> Tested on cris-elf; together with the mentioned update it
> restores build with results consistent with those before the
> breakage.
>
> Ok to commit?
>
> gcc:
>        PR rtl-optimization/49154
>        * ira-costs.c (setup_regno_cost_classes_by_mode): If there
>        already is a matching slot in the hashtable, assign it to
>        classes_ptr.
>

This caused:

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49354
diff mbox

Patch

diff --git gcc/ira-costs.c gcc/ira-costs.c
index f517386..a22bb15 100644
--- gcc/ira-costs.c
+++ gcc/ira-costs.c
@@ -299,6 +299,8 @@  setup_regno_cost_classes_by_mode (int regno, enum machine_mode mode)
 	  classes_ptr = setup_cost_classes (&classes);
 	  *slot = classes_ptr;
 	}
+      else
+	classes_ptr = *slot;
       cost_classes_mode_cache[mode] = (cost_classes_t) *slot;
     }
   regno_cost_classes[regno] = classes_ptr;