[committed] Fix ICE in gen_reg_rtx, at emit-rtl.c:864/865 compiling GNU MPFR

[committed] Fix ICE in gen_reg_rtx, at emit-rtl.c:864/865 compiling GNU MPFR

[John David Anglin]

The attached change fixes PR target/5379.  ICE occurs when reload tries
to emit a move instruction containing a TLS symbol reference as the source
operand.  This requires several scratch registers.  As a result, it isn't
possible for a reload pattern to handle this case.  So, the best solution
was to reject TLS symbol reference source operands after reload starts.

Tested on hppa-unknown-linux-gnu and hppa2.0w-hp-hpux11.11 with not observed
regressions.

Committed to all active branches.

Dave
-- 
J. David Anglin                                  dave.anglin@nrc-cnrc.gc.ca
National Research Council of Canada              (613) 990-0752 (FAX: 952-6602)

2012-12-25  John David Anglin  <dave.anglin@nrc-cnrc.gc.ca>

	PR target/53789
	* config/pa/pa.md (movsi): Reject expansion of TLS symbol references
	after reload starts.

Index: config/pa/pa.md
===================================================================
--- config/pa/pa.md	(revision 194709)
+++ config/pa/pa.md	(working copy)
@@ -2051,6 +2110,12 @@
   ""
   "
 {
+  /* A TLS symbol reference is not a valid move source operand.
+     pa_emit_move_sequence can only handle them prior to reload.
+     There is also no way to reload a TLS symbol reference, so
+     we must reject them after reload starts.  */
+  if (PA_SYMBOL_REF_TLS_P (operands[1]) && !can_create_pseudo_p ())
+    FAIL;
   if (pa_emit_move_sequence (operands, SImode, 0))
     DONE;
 }")

Re: [committed] Fix ICE in gen_reg_rtx, at emit-rtl.c:864/865 compiling GNU MPFR

[Richard Sandiford]

John David Anglin <dave@hiauly1.hia.nrc.ca> writes:
> The attached change fixes PR target/5379.  ICE occurs when reload tries
> to emit a move instruction containing a TLS symbol reference as the source
> operand.  This requires several scratch registers.  As a result, it isn't
> possible for a reload pattern to handle this case.  So, the best solution
> was to reject TLS symbol reference source operands after reload starts.
>
> Tested on hppa-unknown-linux-gnu and hppa2.0w-hp-hpux11.11 with not observed
> regressions.
>
> Committed to all active branches.
>
> Dave
>
> 2012-12-25  John David Anglin  <dave.anglin@nrc-cnrc.gc.ca>
>
> 	PR target/53789
> 	* config/pa/pa.md (movsi): Reject expansion of TLS symbol references
> 	after reload starts.
>
> Index: config/pa/pa.md
> ===================================================================
> --- config/pa/pa.md	(revision 194709)
> +++ config/pa/pa.md	(working copy)
> @@ -2051,6 +2110,12 @@
>    ""
>    "
>  {
> +  /* A TLS symbol reference is not a valid move source operand.
> +     pa_emit_move_sequence can only handle them prior to reload.
> +     There is also no way to reload a TLS symbol reference, so
> +     we must reject them after reload starts.  */
> +  if (PA_SYMBOL_REF_TLS_P (operands[1]) && !can_create_pseudo_p ())
> +    FAIL;
>    if (pa_emit_move_sequence (operands, SImode, 0))
>      DONE;
>  }")

Sorry to butt in, but I'm not sure about this.  Move patterns aren't
supposed to fail.  The nasty thing is that all FAIL actually does is
make the generator return null, and emitting a null insn is a no-op.
That means code like:

  /* from emit_move_insn_1 */
  code = optab_handler (mov_optab, mode);
  if (code != CODE_FOR_nothing)
    return emit_insn (GEN_FCN (code) (x, y));
  ...

  emit_move_insn{,_1} (x, y);

won't generate an ICE on FAIL, but it won't generate any insns either.
If someone's feeling brave, they might want to add an assert for nonnull.

In any case, reload needs to know up-front that the constant can't be
rematerialised, via LEGITIMATE_CONSTANT_P.  emit_move_insn_1 would be
too late.  It looks like PA already handles this for some TLS models:

  /* TLS_MODEL_GLOBAL_DYNAMIC and TLS_MODEL_LOCAL_DYNAMIC are not
     legitimate constants.  */
  if (PA_SYMBOL_REF_TLS_P (x))
   {
     enum tls_model model = SYMBOL_REF_TLS_MODEL (x);

     if (model == TLS_MODEL_GLOBAL_DYNAMIC || model == TLS_MODEL_LOCAL_DYNAMIC)
       return false;
   }

so maybe that should just be:

  if (PA_SYMBOL_REF_TLS_P (x))
    return false;

?  It probably ought to handle symbol + constant too though.

FWIW, I wrote the test below just to make sure that this worked for
mips64-linux-gnu.  OK to install?

...although I just tried it on x86_64-linux-gnu and it ICEs there.

Richard


gcc/testsuite/
	* gcc.dg/torture/tls/tls-reload-1.c: New test.

Index: gcc/testsuite/gcc.dg/torture/tls/tls-reload-1.c
===================================================================
--- /dev/null	2012-12-29 10:15:03.199289441 +0000
+++ gcc/testsuite/gcc.dg/torture/tls/tls-reload-1.c	2013-01-02 12:56:22.278837979 +0000
@@ -0,0 +1,48 @@
+/* { dg-do run } */
+/* { dg-require-effective-target tls_runtime } */
+
+#define ARRAY(X) X##_array
+#define DECLARE(X) \
+  __thread int X; \
+  __thread int ARRAY(X)[4]; \
+  int *volatile *__attribute__((noinline)) \
+  check##X (int *volatile *y) \
+  { \
+    if (!y || *y++ != &X || *y++ != &ARRAY(X)[3]) \
+      return 0; \
+    return y; \
+  }
+#define COPY(X) *y++ = &X; *y++ = &ARRAY(X)[3];
+#define CHECK(X) y = check##X (y);
+#define A(M, X) M(X##0) M(X##1) M(X##2) M(X##3) M(X##4) M(X##5) M(X##6) M(X##7)
+#define B(M, X) A(M, X##0) A(M, X##1) A(M, X##2)
+#define C(M, X) B(M, X) B(M, X) B(M, X)
+
+#define NM 2
+#define NA (NM * 8)
+#define NB (NA * 3)
+#define NC (NB * 3)
+
+extern void abort (void);
+
+B(DECLARE, tls)
+
+void __attribute__ ((noinline))
+setup (int *volatile *y)
+{
+  C(COPY, tls)
+}
+
+int
+main (void)
+{
+  int *volatile array[NC];
+  int *volatile *y = array;
+  int i;
+
+  setup (array);
+  B(CHECK, tls);
+  if (!y)
+    abort ();
+  return 0;
+}

Re: [committed] Fix ICE in gen_reg_rtx, at emit-rtl.c:864/865 compiling GNU MPFR

[Richard Henderson]

On 01/02/2013 05:12 AM, Richard Sandiford wrote:
> 	* gcc.dg/torture/tls/tls-reload-1.c: New test.

Ok.


r~

Re: [committed] Fix ICE in gen_reg_rtx, at emit-rtl.c:864/865 compiling GNU MPFR

[Richard Sandiford]

Richard Henderson <rth@redhat.com> writes:
> On 01/02/2013 05:12 AM, Richard Sandiford wrote:
>> 	* gcc.dg/torture/tls/tls-reload-1.c: New test.
>
> Ok.

Thanks, committed.  And sorry for not volunteering a patch for the x86 ICE,
but I barely know the port...

Richard

Re: [committed] Fix ICE in gen_reg_rtx, at emit-rtl.c:864/865 compiling GNU MPFR

[John David Anglin]

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

Committed the attached change to trunk and 4.7 after testing on hppa- 
unknown-linux-gnu.
Working on a revised change for 4.6.

I don't think symbol + constant can occur but I'm not absolutely sure.

On 2-Jan-13, at 8:12 AM, Richard Sandiford wrote:

> In any case, reload needs to know up-front that the constant can't be
> rematerialised, via LEGITIMATE_CONSTANT_P.  emit_move_insn_1 would be
> too late.  It looks like PA already handles this for some TLS models:
>
>  /* TLS_MODEL_GLOBAL_DYNAMIC and TLS_MODEL_LOCAL_DYNAMIC are not
>     legitimate constants.  */
>  if (PA_SYMBOL_REF_TLS_P (x))
>   {
>     enum tls_model model = SYMBOL_REF_TLS_MODEL (x);
>
>     if (model == TLS_MODEL_GLOBAL_DYNAMIC || model ==  
> TLS_MODEL_LOCAL_DYNAMIC)
>       return false;
>   }
>
> so maybe that should just be:
>
>  if (PA_SYMBOL_REF_TLS_P (x))
>    return false;


Dave
--
John David Anglin	dave.anglin@bell.net


[-- Attachment #2: pa-tls-reload.d.2.txt --]
[-- Type: text/plain, Size: 1530 bytes --]

2013-01-03  John David Anglin  <dave.anglin@nrc-cnrc.gc.ca>

	PR target/53789
	* config/pa/pa.md (movsi): Revert previous change.
	* config/pa/pa.c (pa_legitimate_constant_p): Reject all TLS symbol
	references.

Index: config/pa/pa.md
===================================================================
--- config/pa/pa.md	(revision 194824)
+++ config/pa/pa.md	(working copy)
@@ -2094,12 +2155,6 @@
   ""
   "
 {
-  /* A TLS symbol reference is not a valid move source operand.
-     pa_emit_move_sequence can only handle them prior to reload.
-     There is also no way to reload a TLS symbol reference, so
-     we must reject them after reload starts.  */
-  if (PA_SYMBOL_REF_TLS_P (operands[1]) && !can_create_pseudo_p ())
-    FAIL;
   if (pa_emit_move_sequence (operands, SImode, 0))
     DONE;
 }")
Index: config/pa/pa.c
===================================================================
--- config/pa/pa.c	(revision 194824)
+++ config/pa/pa.c	(working copy)
@@ -10358,15 +10358,11 @@
     return false;
 
   /* TLS_MODEL_GLOBAL_DYNAMIC and TLS_MODEL_LOCAL_DYNAMIC are not
-     legitimate constants.  */
+     legitimate constants.  The other variants can't be handled by
+     the move patterns after reload starts.  */
   if (PA_SYMBOL_REF_TLS_P (x))
-   {
-     enum tls_model model = SYMBOL_REF_TLS_MODEL (x);
+    return false;
 
-     if (model == TLS_MODEL_GLOBAL_DYNAMIC || model == TLS_MODEL_LOCAL_DYNAMIC)
-       return false;
-   }
-
   if (TARGET_64BIT && GET_CODE (x) == CONST_DOUBLE)
     return false;
 

[-- Attachment #3: Type: text/plain, Size: 5 bytes --]

Re: [committed] Fix ICE in gen_reg_rtx, at emit-rtl.c:864/865 compiling GNU MPFR

[David Edelsohn]

The testcase should have included dg-add-options tls.  committed as obvious.

Index: tls-reload-1.c
===================================================================
--- tls-reload-1.c      (revision 194830)
+++ tls-reload-1.c      (working copy)
@@ -1,5 +1,6 @@
 /* { dg-do run } */
 /* { dg-require-effective-target tls_runtime } */
+/* { dg-add-options tls } */

 #define ARRAY(X) X##_array
 #define DECLARE(X) \

Re: [committed] Fix ICE in gen_reg_rtx, at emit-rtl.c:864/865 compiling GNU MPFR

[Richard Sandiford]

David Edelsohn <dje.gcc@gmail.com> writes:
> The testcase should have included dg-add-options tls.  committed as obvious.

Thanks.  I also removed the main point of the test in a final "tweak".
Also committed as obvious after testing on mips64-linux-gnu.

Richard

gcc/testsuite/
	* gcc.dg/torture/tls/tls-reload-1.c (main): Make testing more thorough.

Index: gcc/testsuite/gcc.dg/torture/tls/tls-reload-1.c
===================================================================
--- gcc/testsuite/gcc.dg/torture/tls/tls-reload-1.c	2013-01-03 18:49:20.000000000 +0000
+++ gcc/testsuite/gcc.dg/torture/tls/tls-reload-1.c	2013-01-03 18:49:49.574422889 +0000
@@ -42,7 +42,7 @@ main (void)
   int i;
 
   setup (array);
-  B(CHECK, tls);
+  C(CHECK, tls);
   if (!y)
     abort ();
   return 0;