[PATCH, MIPS]: Fix internal compiler error: in check_bool_attrs, at recog.c:2218 for micromips attribute

[PATCH, MIPS]: Fix internal compiler error: in check_bool_attrs, at recog.c:2218 for micromips attribute

[Robert Suchanek]

Hi,

This patch fixes an internal compiler error when micromips/nomicromips
attributes are used.

The problem here was that the cached boolean attributes for the current
target did not agree with the uncached attributes throwing an assertion error.

It appears that saving and restoring the state for micromips was missing,
just like there is for mips16. OK to apply?

Regards,
Robert

gcc/
	* config/mips/mips.c (micromips_globals): New variable.
	(mips_set_compression_mode): Save and reinitialize target-dependent
	state for microMIPS.

gcc/testsuite
	* gcc.target/mips/umips-attr.c: New test.
---
 gcc/config/mips/mips.c                     | 10 ++++++++++
 gcc/testsuite/gcc.target/mips/umips-attr.c | 13 +++++++++++++
 2 files changed, 23 insertions(+)
 create mode 100644 gcc/testsuite/gcc.target/mips/umips-attr.c

diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c
index bf69850..959a672 100644
--- a/gcc/config/mips/mips.c
+++ b/gcc/config/mips/mips.c
@@ -676,6 +676,9 @@ const char *mips_hi_relocs[NUM_SYMBOL_TYPES];
 /* Target state for MIPS16.  */
 struct target_globals *mips16_globals;
 
+/* Target state for MICROMIPS.  */
+struct target_globals *micromips_globals;
+
 /* Cached value of can_issue_more. This is cached in mips_variable_issue hook
    and returned from mips_sched_reorder2.  */
 static int cached_can_issue_more;
@@ -17182,6 +17185,13 @@ mips_set_compression_mode (unsigned int compression_mode)
       else
 	restore_target_globals (mips16_globals);
     }
+  else if (compression_mode & MASK_MICROMIPS)
+    {
+      if (!micromips_globals)
+	micromips_globals = save_target_globals_default_opts ();
+      else
+	restore_target_globals (micromips_globals);
+    }
   else
     restore_target_globals (&default_target_globals);
 
diff --git a/gcc/testsuite/gcc.target/mips/umips-attr.c b/gcc/testsuite/gcc.target/mips/umips-attr.c
new file mode 100644
index 0000000..f8c4517
--- /dev/null
+++ b/gcc/testsuite/gcc.target/mips/umips-attr.c
@@ -0,0 +1,13 @@
+/* { dg-options "(-mmicromips)" } */
+
+int MICROMIPS
+foo (int a)
+{
+  return a;
+}
+
+int NOMICROMIPS
+foo2 (int a)
+{
+  return a;
+}
-- 
2.2.2

RE: [PATCH, MIPS]: Fix internal compiler error: in check_bool_attrs, at recog.c:2218 for micromips attribute

[Matthew Fortune]

> This patch fixes an internal compiler error when micromips/nomicromips
> attributes are used.
> 
> The problem here was that the cached boolean attributes for the current
> target did not agree with the uncached attributes throwing an assertion
> error.
> 
> It appears that saving and restoring the state for micromips was
> missing, just like there is for mips16. OK to apply?

Just to check, the reason we don't see this in the current testsuite is that
there is no test with both MIPS and microMIPS functions?

> gcc/
> 	* config/mips/mips.c (micromips_globals): New variable.
> 	(mips_set_compression_mode): Save and reinitialize target-dependent
> 	state for microMIPS.
> 
> gcc/testsuite
> 	* gcc.target/mips/umips-attr.c: New test.

OK.

Matthew


> ---
>  gcc/config/mips/mips.c                     | 10 ++++++++++
>  gcc/testsuite/gcc.target/mips/umips-attr.c | 13 +++++++++++++
>  2 files changed, 23 insertions(+)
>  create mode 100644 gcc/testsuite/gcc.target/mips/umips-attr.c
> 
> diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c index
> bf69850..959a672 100644
> --- a/gcc/config/mips/mips.c
> +++ b/gcc/config/mips/mips.c
> @@ -676,6 +676,9 @@ const char *mips_hi_relocs[NUM_SYMBOL_TYPES];
>  /* Target state for MIPS16.  */
>  struct target_globals *mips16_globals;
> 
> +/* Target state for MICROMIPS.  */
> +struct target_globals *micromips_globals;
> +
>  /* Cached value of can_issue_more. This is cached in
> mips_variable_issue hook
>     and returned from mips_sched_reorder2.  */  static int
> cached_can_issue_more; @@ -17182,6 +17185,13 @@
> mips_set_compression_mode (unsigned int compression_mode)
>        else
>  	restore_target_globals (mips16_globals);
>      }
> +  else if (compression_mode & MASK_MICROMIPS)
> +    {
> +      if (!micromips_globals)
> +	micromips_globals = save_target_globals_default_opts ();
> +      else
> +	restore_target_globals (micromips_globals);
> +    }
>    else
>      restore_target_globals (&default_target_globals);
> 
> diff --git a/gcc/testsuite/gcc.target/mips/umips-attr.c
> b/gcc/testsuite/gcc.target/mips/umips-attr.c
> new file mode 100644
> index 0000000..f8c4517
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/mips/umips-attr.c
> @@ -0,0 +1,13 @@
> +/* { dg-options "(-mmicromips)" } */
> +
> +int MICROMIPS
> +foo (int a)
> +{
> +  return a;
> +}
> +
> +int NOMICROMIPS
> +foo2 (int a)
> +{
> +  return a;
> +}
> --
> 2.2.2
> 

RE: [PATCH, MIPS]: Fix internal compiler error: in check_bool_attrs, at recog.c:2218 for micromips attribute

[Robert Suchanek]

Hi Matthew,

> > This patch fixes an internal compiler error when micromips/nomicromips
> > attributes are used.
> >
> > The problem here was that the cached boolean attributes for the current
> > target did not agree with the uncached attributes throwing an assertion
> > error.
> >
> > It appears that saving and restoring the state for micromips was
> > missing, just like there is for mips16. OK to apply?
> 
> Just to check, the reason we don't see this in the current testsuite is that
> there is no test with both MIPS and microMIPS functions?

Correct.  The testsuite either tests microMIPS or MIPS but not both in the same TU.

> > gcc/
> > 	* config/mips/mips.c (micromips_globals): New variable.
> > 	(mips_set_compression_mode): Save and reinitialize target-dependent
> > 	state for microMIPS.
> >
> > gcc/testsuite
> > 	* gcc.target/mips/umips-attr.c: New test.
> 
> OK.

Committed as r223294.

Regards,
Robert

RE: [PATCH, MIPS]: Fix internal compiler error: in check_bool_attrs, at recog.c:2218 for micromips attribute

[Maciej W. Rozycki]

On Mon, 18 May 2015, Robert Suchanek wrote:

> > Just to check, the reason we don't see this in the current testsuite is that
> > there is no test with both MIPS and microMIPS functions?
> 
> Correct.  The testsuite either tests microMIPS or MIPS but not both in 
> the same TU.

 We could add -mflip-micromips complementing -mflip-mips16 and use that 
for testing too.  Chances are it'd reveal further issues.  Looking at how 
-mflip-mips16 has been implemented it does not appear to me adding 
-mflip-micromips would be a lot of effort.

  Maciej

RE: [PATCH, MIPS]: Fix internal compiler error: in check_bool_attrs, at recog.c:2218 for micromips attribute

[Robert Suchanek]

Hi,

The original patch had a missing declaration of micromips_globals in mips.h 
that appears to be the cause of segmentation faults when building mips-mti-linux-gnu.
I didn't get any failures just before the submission neither on mips-img-linux-gnu
nor mips64el-linux-gnu and the test case is too trivial to trigger the ICE.

Below is the missing line. With this change mips-mti-linux-gnu builds fine.
The trunk is unstable and needed another patch from PR66181 to build
Glibc. Ok to commit?

>  We could add -mflip-micromips complementing -mflip-mips16 and use that
> for testing too.  Chances are it'd reveal further issues.  Looking at how
> -mflip-mips16 has been implemented it does not appear to me adding
> -mflip-micromips would be a lot of effort.

I'm in favour of adding such a switch since the testsuite doesn't cover 
a mixture of MIPS and microMIPS code.

Regards,
Robert

gcc/
	* config/mips/mips.h (micromips_globals): Declare.
---
 gcc/config/mips/mips.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/gcc/config/mips/mips.h b/gcc/config/mips/mips.h
index 0ea4e6d..85c8a97 100644
--- a/gcc/config/mips/mips.h
+++ b/gcc/config/mips/mips.h
@@ -3108,6 +3108,7 @@ extern const struct mips_cpu_info *mips_arch_info;
 extern const struct mips_cpu_info *mips_tune_info;
 extern unsigned int mips_base_compression_flags;
 extern GTY(()) struct target_globals *mips16_globals;
+extern GTY(()) struct target_globals *micromips_globals;
 #endif
 
 /* Enable querying of DFA units.  */
-- 
2.2.2

RE: [PATCH, MIPS]: Fix internal compiler error: in check_bool_attrs, at recog.c:2218 for micromips attribute

[Matthew Fortune]

> >  We could add -mflip-micromips complementing -mflip-mips16 and use
> > that for testing too.  Chances are it'd reveal further issues.
> > Looking at how
> > -mflip-mips16 has been implemented it does not appear to me adding
> > -mflip-micromips would be a lot of effort.
> 
> I'm in favour of adding such a switch since the testsuite doesn't cover
> a mixture of MIPS and microMIPS code.

It certainly seems that we need a bit more coverage here in order that
we can mostly stick to testing one or two MIPS configurations per commit.

We'll have some MIPS machines in the compile farm shortly which may allow
us to at least do the full all-config build of the toolchain more easily
even if that doesn't extend to testing all the configs.

> 
> Regards,
> Robert
> 
> gcc/
> 	* config/mips/mips.h (micromips_globals): Declare.

OK, thanks.

Matthew

RE: [PATCH, MIPS]: Fix internal compiler error: in check_bool_attrs, at recog.c:2218 for micromips attribute

[Robert Suchanek]

> > gcc/
> > 	* config/mips/mips.h (micromips_globals): Declare.
> 
> OK, thanks.
> 
> Matthew

Committed as r223438.

Robert