[PATCH] Fix bootstrap fail on spu vs. ira.c/genpreds

[PATCH] Fix bootstrap fail on spu vs. ira.c/genpreds

[Dave Korn]

    Hello all,


  As discussed on the main list earlier today[*], there is currently an
implicit assumption in ira.c that every backend implements at least one
define_register_constraint; when the backend doesn't, genpreds does not output
any definition for regclass_for_constraint and then ira.c (which refers to it
unguarded) fails to link.


gcc/ChangeLog

	* ira.c (setup_cover_and_important_classes):  Use safe macro
	REG_CLASS_FOR_CONSTRAINT instead of calling regclass_for_constraint
	directly.
	* genpreds.c (write_tm_preds_h):  Output suitable definition of
	REG_CLASS_FOR_CONSTRAINT.

  The attached patch fixes the reported bug according to Revital; I'm putting
it through a bootstrap right now (all languages except for Ada), on
i686-pc-cygwin, which is a platform that won't hit the new code paths, so can
I skip running the testsuites?  Ok for trunk?

    cheers,
      DaveK
-- 
[*] - http://gcc.gnu.org/ml/gcc/2009-04/threads.html#00696

Re: [PATCH] Fix bootstrap fail on spu vs. ira.c/genpreds

[Vladimir Makarov]

Dave Korn wrote:
>     Hello all,
>
>
>   As discussed on the main list earlier today[*], there is currently an
> implicit assumption in ira.c that every backend implements at least one
> define_register_constraint; when the backend doesn't, genpreds does not output
> any definition for regclass_for_constraint and then ira.c (which refers to it
> unguarded) fails to link.
>
>
> gcc/ChangeLog
>
> 	* ira.c (setup_cover_and_important_classes):  Use safe macro
> 	REG_CLASS_FOR_CONSTRAINT instead of calling regclass_for_constraint
> 	directly.
> 	* genpreds.c (write_tm_preds_h):  Output suitable definition of
> 	REG_CLASS_FOR_CONSTRAINT.
>
>   The attached patch fixes the reported bug according to Revital; I'm putting
> it through a bootstrap right now (all languages except for Ada), on
> i686-pc-cygwin, which is a platform that won't hit the new code paths, so can
> I skip running the testsuites?  Ok for trunk?
>
>   
The patch posted in

http://gcc.gnu.org/ml/gcc/2009-04/msg00698.html

is done for older version of ira.c.  In the change context for ira.c 
instead of

 #ifdef CONSTRAINT__LIMIT

should be

#ifdef CONSTRAINT_NUM_DEFINED_P


Other than that the patch is ok.  Although, personally, I'd prefer the following
patch (because it does not create a new macro which is used only in one place
of the compiler):

2009-04-27  Vladimir Makarov  <vmakarov@redhat.com>

	* genpreds.c (write_enum_constraint_num): Output
	REG_CONSTRAINT_DEFINED_P instead of CONSTRAINT_NUM_DEFINED_P and
	only if there are register constraints.
	* ira.c (setup_cover_and_important_classes): Use
	REG_CONSTRAINT_DEFINED_P instead of CONSTRAINT_NUM_DEFINED_P.

Index: genpreds.c
===================================================================
--- genpreds.c	(revision 146793)
+++ genpreds.c	(working copy)
@@ -954,7 +954,8 @@ write_enum_constraint_num (void)
 {
   struct constraint_data *c;
 
-  fputs ("#define CONSTRAINT_NUM_DEFINED_P 1\n", stdout);
+  if (have_register_constraints)
+    fputs ("#define REG_CONSTRAINT_DEFINED_P 1\n", stdout);
   fputs ("enum constraint_num\n"
 	 "{\n"
 	 "  CONSTRAINT__UNKNOWN = 0", stdout);
Index: ira.c
===================================================================
--- ira.c	(revision 146793)
+++ ira.c	(working copy)
@@ -754,7 +754,7 @@ setup_cover_and_important_classes (void)
 	{
 	  if (i == NO_REGS)
 	    continue;
-#ifdef CONSTRAINT_NUM_DEFINED_P
+#ifdef REG_CONSTRAINT_DEFINED_P
 	  for (j = 0; j < CONSTRAINT__LIMIT; j++)
 	    if ((int) regclass_for_constraint ((enum constraint_num) j) == i)
 	      break;

Re: [PATCH] Fix bootstrap fail on spu vs. ira.c/genpreds

[Dave Korn]

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

Vladimir Makarov wrote:
>> gcc/ChangeLog
>>
>>     * ira.c (setup_cover_and_important_classes):  Use safe macro
>>     REG_CLASS_FOR_CONSTRAINT instead of calling regclass_for_constraint
>>     directly.
>>     * genpreds.c (write_tm_preds_h):  Output suitable definition of
>>     REG_CLASS_FOR_CONSTRAINT.

> The patch posted in
> 
> http://gcc.gnu.org/ml/gcc/2009-04/msg00698.html
> 
> is done for older version of ira.c.

  Oops, yes; I updated it before I started the build, here's the current version.

> Other than that the patch is ok.  Although, personally, I'd prefer the 
> following patch (because it does not create a new macro which is used only
> in one place of the compiler):

  Your solution is clean, but if anyone ever wants to call
regclass_for_constraint again in the future we might run into the same problem
again, might we not?  I don't really mind which patch goes in, it's your call.

    cheers,
      DaveK



[-- Attachment #2: r146825-patch.diff --]
[-- Type: text/x-c, Size: 1443 bytes --]

Index: gcc/ira.c
===================================================================
--- gcc/ira.c	(revision 146838)
+++ gcc/ira.c	(working copy)
@@ -756,7 +756,7 @@ setup_cover_and_important_classes (void)
 	    continue;
 #ifdef CONSTRAINT_NUM_DEFINED_P
 	  for (j = 0; j < CONSTRAINT__LIMIT; j++)
-	    if ((int) regclass_for_constraint ((enum constraint_num) j) == i)
+	    if ((int) REG_CLASS_FOR_CONSTRAINT ((enum constraint_num) j) == i)
 	      break;
 	  if (j < CONSTRAINT__LIMIT)
 	    {
Index: gcc/genpreds.c
===================================================================
--- gcc/genpreds.c	(revision 146838)
+++ gcc/genpreds.c	(working copy)
@@ -1280,9 +1280,13 @@ write_tm_preds_h (void)
 	puts ("extern enum reg_class regclass_for_constraint "
 	      "(enum constraint_num);\n"
 	      "#define REG_CLASS_FROM_CONSTRAINT(c_,s_) \\\n"
-	      "    regclass_for_constraint (lookup_constraint (s_))\n");
+	      "    regclass_for_constraint (lookup_constraint (s_))\n"
+	      "#define REG_CLASS_FOR_CONSTRAINT(x_) \\\n"
+	      "    regclass_for_constraint (x_)\n");
       else
-	puts ("#define REG_CLASS_FROM_CONSTRAINT(c_,s_) NO_REGS");
+	puts ("#define REG_CLASS_FROM_CONSTRAINT(c_,s_) NO_REGS\n"
+	      "#define REG_CLASS_FOR_CONSTRAINT(x_) \\\n"
+	      "    NO_REGS\n");
       if (have_const_int_constraints)
 	puts ("extern bool insn_const_int_ok_for_constraint "
 	      "(HOST_WIDE_INT, enum constraint_num);\n"

Re: [PATCH] Fix bootstrap fail on spu vs. ira.c/genpreds

[Vladimir Makarov]

Dave Korn wrote:
> Vladimir Makarov wrote:
>   
>>> gcc/ChangeLog
>>>
>>>     * ira.c (setup_cover_and_important_classes):  Use safe macro
>>>     REG_CLASS_FOR_CONSTRAINT instead of calling regclass_for_constraint
>>>     directly.
>>>     * genpreds.c (write_tm_preds_h):  Output suitable definition of
>>>     REG_CLASS_FOR_CONSTRAINT.
>>>       
>
>   
>> The patch posted in
>>
>> http://gcc.gnu.org/ml/gcc/2009-04/msg00698.html
>>
>> is done for older version of ira.c.
>>     
>
>   Oops, yes; I updated it before I started the build, here's the current version.
>
>   
>> Other than that the patch is ok.  Although, personally, I'd prefer the 
>> following patch (because it does not create a new macro which is used only
>> in one place of the compiler):
>>     
>
>   Your solution is clean, but if anyone ever wants to call
> regclass_for_constraint again in the future we might run into the same problem
> again, might we not?  I don't really mind which patch goes in, it's your call.
>
>   
There is a sense in your words too.  So on the second thought, I'd go 
for your patch.  Unfortunately for this case, I am not a maintainer of 
this part of compiler.  So my ok is not an approval for committing the 
patch.  I think Ian could do that.  I am CCing the email to him.

In any case thanks for the patch, Dave.

Re: [PATCH] Fix bootstrap fail on spu vs. ira.c/genpreds

[Jakub Jelinek]

On Mon, Apr 27, 2009 at 12:36:52PM -0400, Vladimir Makarov wrote:
>>   Your solution is clean, but if anyone ever wants to call
>> regclass_for_constraint again in the future we might run into the same problem
>> again, might we not?  I don't really mind which patch goes in, it's your call.
>>
>>   
> There is a sense in your words too.  So on the second thought, I'd go  
> for your patch.  Unfortunately for this case, I am not a maintainer of  
> this part of compiler.  So my ok is not an approval for committing the  
> patch.  I think Ian could do that.  I am CCing the email to him.

Wouldn't it be better to just
#define regclass_for_constraint(x) NO_REGS
then in the generated header in that case?

	Jakub

Re: [PATCH] Fix bootstrap fail on spu vs. ira.c/genpreds

[Vladimir Makarov]

Jakub Jelinek wrote:
> On Mon, Apr 27, 2009 at 12:36:52PM -0400, Vladimir Makarov wrote:
>   
>>>   Your solution is clean, but if anyone ever wants to call
>>> regclass_for_constraint again in the future we might run into the same problem
>>> again, might we not?  I don't really mind which patch goes in, it's your call.
>>>
>>>   
>>>       
>> There is a sense in your words too.  So on the second thought, I'd go  
>> for your patch.  Unfortunately for this case, I am not a maintainer of  
>> this part of compiler.  So my ok is not an approval for committing the  
>> patch.  I think Ian could do that.  I am CCing the email to him.
>>     
>
> Wouldn't it be better to just
> #define regclass_for_constraint(x) NO_REGS
> then in the generated header in that case?
>
>   
It could be better but as I remember macros should be written in upper 
case according to GNU standards and functions in lower case.  INHO, it 
means you can not define object as macro in one case and function in 
another case.

Re: [PATCH] Fix bootstrap fail on spu vs. ira.c/genpreds

[Dave Korn]

Jakub Jelinek wrote:
> On Mon, Apr 27, 2009 at 12:36:52PM -0400, Vladimir Makarov wrote:
>>>   Your solution is clean, but if anyone ever wants to call
>>> regclass_for_constraint again in the future we might run into the same problem
>>> again, might we not?  I don't really mind which patch goes in, it's your call.
>>>
>>>   
>> There is a sense in your words too.  So on the second thought, I'd go  
>> for your patch.  Unfortunately for this case, I am not a maintainer of  
>> this part of compiler.  So my ok is not an approval for committing the  
>> patch.  I think Ian could do that.  I am CCing the email to him.
> 
> Wouldn't it be better to just
> #define regclass_for_constraint(x) NO_REGS
> then in the generated header in that case?

  I was just being consistent with the way the other functions in tm-preds.h
get macro wrappers.  I didn't find any direct (unwrapped) calls of a handful
of the functions that I grepped for in the rest of the source, so I think it's
a convention to only access those functions through the wrappers, broken by
(as far as I could tell) just this single instance in ira.c.

    cheers,
      DaveK

Re: [PATCH] Fix bootstrap fail on spu vs. ira.c/genpreds

[Anthony Green]

Dave Korn wrote:
>     Hello all,
>
>
>   As discussed on the main list earlier today[*], there is currently an
> implicit assumption in ira.c that every backend implements at least one
> define_register_constraint; when the backend doesn't, genpreds does not output
> any definition for regclass_for_constraint and then ira.c (which refers to it
> unguarded) fails to link.
>
>
> gcc/ChangeLog
>
> 	* ira.c (setup_cover_and_important_classes):  Use safe macro
> 	REG_CLASS_FOR_CONSTRAINT instead of calling regclass_for_constraint
> 	directly.
> 	* genpreds.c (write_tm_preds_h):  Output suitable definition of
> 	REG_CLASS_FOR_CONSTRAINT.
>
>   The attached patch fixes the reported bug according to Revital; I'm putting
> it through a bootstrap right now (all languages except for Ada), on
> i686-pc-cygwin, which is a platform that won't hit the new code paths, so can
> I skip running the testsuites?  Ok for trunk?
>
>     cheers,
>       DaveK
>   
This patch is also needed for the moxie port, should anybody be looking 
at reviewing it before this fix goes in.

  http://gcc.gnu.org/ml/gcc-patches/2009-04/msg02105.html

Thanks,

AG

Re: [PATCH] Fix bootstrap fail on spu vs. ira.c/genpreds

[Michael Meissner]

On Mon, Apr 27, 2009 at 03:52:11PM +0100, Dave Korn wrote:
> 
>     Hello all,
> 
> 
>   As discussed on the main list earlier today[*], there is currently an
> implicit assumption in ira.c that every backend implements at least one
> define_register_constraint; when the backend doesn't, genpreds does not output
> any definition for regclass_for_constraint and then ira.c (which refers to it
> unguarded) fails to link.
> 
> 
> gcc/ChangeLog
> 
> 	* ira.c (setup_cover_and_important_classes):  Use safe macro
> 	REG_CLASS_FOR_CONSTRAINT instead of calling regclass_for_constraint
> 	directly.
> 	* genpreds.c (write_tm_preds_h):  Output suitable definition of
> 	REG_CLASS_FOR_CONSTRAINT.
> 
>   The attached patch fixes the reported bug according to Revital; I'm putting
> it through a bootstrap right now (all languages except for Ada), on
> i686-pc-cygwin, which is a platform that won't hit the new code paths, so can
> I skip running the testsuites?  Ok for trunk?
> 
>     cheers,
>       DaveK
> -- 
> [*] - http://gcc.gnu.org/ml/gcc/2009-04/threads.html#00696

I applied this patch to the mainline.  Thanks for doing this.

Vlad, if you would prefer to submit a new patch to eliminate the
REG_CLASS_FOR_CONSTRAINT macro as you discussed in the replies, I would have no
problems with that either.

-- 
Michael Meissner, IBM
4 Technology Place Drive, MS 2203A, Westford, MA, 01886, USA
meissner@linux.vnet.ibm.com