[PATCH v3] S/390: Allow relative addressing of literal pool entries

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH v3] S/390: Allow relative addressing of literal pool entries

Ilya Leoshkevich
Bootstrapped and regtested on s390x-redhat-linux.

r265490 allowed the compiler to choose in a more flexible way whether to
use load or load-address-relative-long (LARL) instruction.  When it
chose LARL for literal pool references, the latter ones were rewritten
by pass_s390_early_mach to use UNSPEC_LTREF, which assumes base register
usage, which in turn is not compatible with LARL.  The end result was an
ICE because of unrecognizable insn.

UNSPEC_LTREF and friends are necessary in order to communicate the
dependency on the base register to pass_sched2.  When relative
addressing is used, no base register is necessary, so in such cases the
rewrite must be avoided.

gcc/ChangeLog:

2018-11-05  Ilya Leoshkevich  <[hidden email]>

        PR target/87762
        * config/s390/predicates.md (larl_operand): Use
        s390_symbol_relative_long_p () to reduce code duplication.
        * config/s390/s390-protos.h (s390_symbol_relative_long_p): New
        function.
        * config/s390/s390.c (s390_safe_relative_long_p): Likewise.
        (annotate_constant_pool_refs): Skip insns which support relative
        addressing.
        (annotate_constant_pool_refs_1): New helper function.
        (s390_symbol_relative_long_p): New function.
        (find_constant_pool_ref): Return unannotated constant pool
        references.
        (replace_constant_pool_ref): Skip insns which support relative
        addressing.
        (replace_constant_pool_ref_1): New helper function.
        (s390_mainpool_finish): Adjust to the new signature of
        replace_constant_pool_ref ().
        (s390_chunkify_finish): Likewise.
        (pass_s390_early_mach::execute): Likewise.
        (s390_prologue_plus_offset): Likewise.
        (s390_emit_prologue): Likewise.
        (s390_emit_epilogue): Likewise.
---
 gcc/config/s390/predicates.md |   8 +--
 gcc/config/s390/s390-protos.h |   1 +
 gcc/config/s390/s390.c        | 107 ++++++++++++++++++++++++++--------
 3 files changed, 85 insertions(+), 31 deletions(-)

diff --git a/gcc/config/s390/predicates.md b/gcc/config/s390/predicates.md
index 97f717c558d..bb66c8a6bcb 100644
--- a/gcc/config/s390/predicates.md
+++ b/gcc/config/s390/predicates.md
@@ -151,9 +151,7 @@
   if (GET_CODE (op) == LABEL_REF)
     return true;
   if (SYMBOL_REF_P (op))
-    return (!SYMBOL_FLAG_NOTALIGN2_P (op)
-    && SYMBOL_REF_TLS_MODEL (op) == 0
-    && s390_rel_address_ok_p (op));
+    return s390_symbol_relative_long_p (op);
 
   /* Everything else must have a CONST, so strip it.  */
   if (GET_CODE (op) != CONST)
@@ -176,9 +174,7 @@
   if (GET_CODE (op) == LABEL_REF)
     return true;
   if (SYMBOL_REF_P (op))
-    return (!SYMBOL_FLAG_NOTALIGN2_P (op)
-    && SYMBOL_REF_TLS_MODEL (op) == 0
-    && s390_rel_address_ok_p (op));
+    return s390_symbol_relative_long_p (op);
 
 
   /* Now we must have a @GOTENT offset or @PLT stub
diff --git a/gcc/config/s390/s390-protos.h b/gcc/config/s390/s390-protos.h
index 96fa705f879..d3c2cc55e28 100644
--- a/gcc/config/s390/s390-protos.h
+++ b/gcc/config/s390/s390-protos.h
@@ -157,6 +157,7 @@ extern void s390_indirect_branch_via_thunk (unsigned int regno,
     rtx comparison_operator,
     enum s390_indirect_branch_type type);
 extern void s390_indirect_branch_via_inline_thunk (rtx execute_target);
+extern bool s390_symbol_relative_long_p (rtx);
 #endif /* RTX_CODE */
 
 /* s390-c.c routines */
diff --git a/gcc/config/s390/s390.c b/gcc/config/s390/s390.c
index 1be85727b73..c1318c25004 100644
--- a/gcc/config/s390/s390.c
+++ b/gcc/config/s390/s390.c
@@ -2731,6 +2731,17 @@ s390_safe_attr_type (rtx_insn *insn)
     return TYPE_NONE;
 }
 
+/* Return attribute relative_long of insn.  */
+
+static bool
+s390_safe_relative_long_p (rtx_insn *insn)
+{
+  if (recog_memoized (insn) >= 0)
+    return get_attr_relative_long (insn) == RELATIVE_LONG_YES;
+  else
+    return false;
+}
+
 /* Return true if DISP is a valid short displacement.  */
 
 static bool
@@ -8116,11 +8127,8 @@ s390_first_cycle_multipass_dfa_lookahead (void)
   return 4;
 }
 
-/* Annotate every literal pool reference in X by an UNSPEC_LTREF expression.
-   Fix up MEMs as required.  */
-
 static void
-annotate_constant_pool_refs (rtx *x)
+annotate_constant_pool_refs_1 (rtx *x)
 {
   int i, j;
   const char *fmt;
@@ -8199,18 +8207,41 @@ annotate_constant_pool_refs (rtx *x)
     {
       if (fmt[i] == 'e')
  {
-  annotate_constant_pool_refs (&XEXP (*x, i));
+  annotate_constant_pool_refs_1 (&XEXP (*x, i));
  }
       else if (fmt[i] == 'E')
  {
   for (j = 0; j < XVECLEN (*x, i); j++)
-    annotate_constant_pool_refs (&XVECEXP (*x, i, j));
+    annotate_constant_pool_refs_1 (&XVECEXP (*x, i, j));
  }
     }
 }
 
-/* Find an annotated literal pool symbol referenced in RTX X,
-   and store it at REF.  Will abort if X contains references to
+/* Annotate every literal pool reference in INSN by an UNSPEC_LTREF expression.
+   Fix up MEMs as required.
+   Skip insns which support relative addressing, because they do not use a base
+   register.  */
+
+static void
+annotate_constant_pool_refs (rtx_insn *insn)
+{
+  if (s390_safe_relative_long_p (insn))
+    return;
+  annotate_constant_pool_refs_1 (&PATTERN (insn));
+}
+
+/* Return true iff SYMBOL_REF X can be used with a relative long addressing. */
+
+bool
+s390_symbol_relative_long_p (rtx x)
+{
+  return (!SYMBOL_FLAG_NOTALIGN2_P (x)
+  && SYMBOL_REF_TLS_MODEL (x) == 0
+  && s390_rel_address_ok_p (x));
+}
+
+/* Find a literal pool symbol referenced in INSN,
+   and store it at REF.  Will abort if INSN contains references to
    more than one such pool symbol; multiple references to the same
    symbol are allowed, however.
 
@@ -8228,6 +8259,23 @@ find_constant_pool_ref (rtx x, rtx *ref)
       && XINT (x, 1) == UNSPECV_POOL_ENTRY)
     return;
 
+  /* Return unannotated constant pool references, so that the corresponding
+     entries are added to the back-end-managed pool.  Not doing so would result
+     in those entries being placed in the middle-end-managed pool, which would
+     in turn prevent merging them with identical ones in the back-end-managed
+     pool.  */
+  if (SYMBOL_REF_P (x)
+      && CONSTANT_POOL_ADDRESS_P (x)
+      && s390_symbol_relative_long_p (x))
+    {
+      if (*ref == NULL_RTX)
+ *ref = x;
+      else
+ gcc_assert (*ref == x);
+
+      return;
+    }
+
   gcc_assert (GET_CODE (x) != SYMBOL_REF
       || !CONSTANT_POOL_ADDRESS_P (x));
 
@@ -8260,11 +8308,8 @@ find_constant_pool_ref (rtx x, rtx *ref)
     }
 }
 
-/* Replace every reference to the annotated literal pool
-   symbol REF in X by its base plus OFFSET.  */
-
 static void
-replace_constant_pool_ref (rtx *x, rtx ref, rtx offset)
+replace_constant_pool_ref_1 (rtx *x, rtx ref, rtx offset)
 {
   int i, j;
   const char *fmt;
@@ -8295,16 +8340,29 @@ replace_constant_pool_ref (rtx *x, rtx ref, rtx offset)
     {
       if (fmt[i] == 'e')
  {
-  replace_constant_pool_ref (&XEXP (*x, i), ref, offset);
+  replace_constant_pool_ref_1 (&XEXP (*x, i), ref, offset);
  }
       else if (fmt[i] == 'E')
  {
   for (j = 0; j < XVECLEN (*x, i); j++)
-    replace_constant_pool_ref (&XVECEXP (*x, i, j), ref, offset);
+    replace_constant_pool_ref_1 (&XVECEXP (*x, i, j), ref, offset);
  }
     }
 }
 
+/* Replace every reference to the annotated literal pool
+   symbol REF in INSN by its base plus OFFSET.
+   Skip insns which support relative addressing, because they do not use a base
+   register.  */
+
+static void
+replace_constant_pool_ref (rtx_insn *insn, rtx ref, rtx offset)
+{
+  if (s390_safe_relative_long_p (insn))
+    return;
+  replace_constant_pool_ref_1 (&PATTERN (insn), ref, offset);
+}
+
 /* We keep a list of constants which we have to add to internal
    constant tables in the middle of large functions.  */
 
@@ -8801,7 +8859,7 @@ s390_mainpool_finish (struct constant_pool *pool)
  addr = s390_find_constant (pool, get_pool_constant (pool_ref),
  get_pool_mode (pool_ref));
 
-      replace_constant_pool_ref (&PATTERN (insn), pool_ref, addr);
+      replace_constant_pool_ref (insn, pool_ref, addr);
       INSN_CODE (insn) = -1;
     }
  }
@@ -9002,7 +9060,7 @@ s390_chunkify_finish (struct constant_pool *pool_list)
    get_pool_constant (pool_ref),
    get_pool_mode (pool_ref));
 
-      replace_constant_pool_ref (&PATTERN (insn), pool_ref, addr);
+      replace_constant_pool_ref (insn, pool_ref, addr);
       INSN_CODE (insn) = -1;
     }
  }
@@ -10595,7 +10653,7 @@ pass_s390_early_mach::execute (function *fun)
   for (insn = get_insns (); insn; insn = NEXT_INSN (insn))
     if (INSN_P (insn))
       {
- annotate_constant_pool_refs (&PATTERN (insn));
+ annotate_constant_pool_refs (insn);
  df_insn_rescan (insn);
       }
   return 0;
@@ -10616,7 +10674,7 @@ make_pass_s390_early_mach (gcc::context *ctxt)
 static rtx
 s390_prologue_plus_offset (rtx target, rtx reg, rtx offset, bool frame_related_p)
 {
-  rtx insn;
+  rtx_insn *insn;
   rtx orig_offset = offset;
 
   gcc_assert (REG_P (target));
@@ -10650,7 +10708,7 @@ s390_prologue_plus_offset (rtx target, rtx reg, rtx offset, bool frame_related_p
 
       if (!CONST_INT_P (offset))
  {
-  annotate_constant_pool_refs (&PATTERN (insn));
+  annotate_constant_pool_refs (insn);
 
   if (frame_related_p)
     add_reg_note (insn, REG_FRAME_RELATED_EXPR,
@@ -11090,7 +11148,7 @@ s390_emit_prologue (void)
       rtx_insn *insns = s390_load_got ();
 
       for (rtx_insn *insn = insns; insn; insn = NEXT_INSN (insn))
- annotate_constant_pool_refs (&PATTERN (insn));
+ annotate_constant_pool_refs (insn);
 
       emit_insn (insns);
     }
@@ -11154,7 +11212,8 @@ s390_emit_epilogue (bool sibcall)
     }
   else
     {
-      rtx insn, frame_off, cfa;
+      rtx_insn *insn;
+      rtx frame_off, cfa;
 
       offset = area_bottom < 0 ? -area_bottom : 0;
       frame_off = GEN_INT (cfun_frame_layout.frame_size - offset);
@@ -11163,9 +11222,7 @@ s390_emit_epilogue (bool sibcall)
  gen_rtx_PLUS (Pmode, frame_pointer, frame_off));
       if (DISP_IN_RANGE (INTVAL (frame_off)))
  {
-  insn = gen_rtx_SET (frame_pointer,
-      gen_rtx_PLUS (Pmode, frame_pointer, frame_off));
-  insn = emit_insn (insn);
+  insn = emit_insn (copy_rtx (cfa));
  }
       else
  {
@@ -11173,7 +11230,7 @@ s390_emit_epilogue (bool sibcall)
     frame_off = force_const_mem (Pmode, frame_off);
 
   insn = emit_insn (gen_add2_insn (frame_pointer, frame_off));
-  annotate_constant_pool_refs (&PATTERN (insn));
+  annotate_constant_pool_refs (insn);
  }
       add_reg_note (insn, REG_CFA_ADJUST_CFA, cfa);
       RTX_FRAME_RELATED_P (insn) = 1;
--
2.19.1

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3] S/390: Allow relative addressing of literal pool entries

Ulrich Weigand
Ilya Leoshkevich wrote:

> +  /* Return unannotated constant pool references, so that the corresponding
> +     entries are added to the back-end-managed pool.  Not doing so would result
> +     in those entries being placed in the middle-end-managed pool, which would
> +     in turn prevent merging them with identical ones in the back-end-managed
> +     pool.  */

I'm still not convinced this is necessary; how frequently does the case occur
that a constant is duplicated, and is it worth the extra code complexity?
(Also, note that the middle-end pool is shared across the whole translation
unit, while the back-end pool is duplicated in every function ... so if the
same constant is used by many functions, forcing it into the back-end pool
may *increase* overall size.)

In any case, does this even happen with your current code?  You find the
unannotated reference here, and therefore copy the constant into the local
pool, but then replace_constant_pool_ref will still ignore the insn, and
thus the insn will actually continue to refer to the middle-end pool
constant, *not* the copy created in the local pool.  Right?

>        if (DISP_IN_RANGE (INTVAL (frame_off)))
>   {
> -  insn = gen_rtx_SET (frame_pointer,
> -      gen_rtx_PLUS (Pmode, frame_pointer, frame_off));
> -  insn = emit_insn (insn);
> +  insn = emit_insn (copy_rtx (cfa));
>   }

This seems unrelated?

Bye,
Ulrich

--
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3] S/390: Allow relative addressing of literal pool entries

Ilya Leoshkevich
> Am 09.11.2018 um 14:43 schrieb Ulrich Weigand <[hidden email]>:
>
> Ilya Leoshkevich wrote:
>
>> +  /* Return unannotated constant pool references, so that the corresponding
>> +     entries are added to the back-end-managed pool.  Not doing so would result
>> +     in those entries being placed in the middle-end-managed pool, which would
>> +     in turn prevent merging them with identical ones in the back-end-managed
>> +     pool.  */
>
> I'm still not convinced this is necessary; how frequently does the case occur
> that a constant is duplicated, and is it worth the extra code complexity?
> (Also, note that the middle-end pool is shared across the whole translation
> unit, while the back-end pool is duplicated in every function ... so if the
> same constant is used by many functions, forcing it into the back-end pool
> may *increase* overall size.)
>
> In any case, does this even happen with your current code?  You find the
> unannotated reference here, and therefore copy the constant into the local
> pool, but then replace_constant_pool_ref will still ignore the insn, and
> thus the insn will actually continue to refer to the middle-end pool
> constant, *not* the copy created in the local pool.  Right?

Whoops, that’s right.  This code must be currently achieving nothing at
all!  I did not run into such cases and I don’t have a test case for
them, so they are hypothetical.  So let’s keep it simple here.

>
>>       if (DISP_IN_RANGE (INTVAL (frame_off)))
>> {
>> -  insn = gen_rtx_SET (frame_pointer,
>> -      gen_rtx_PLUS (Pmode, frame_pointer, frame_off));
>> -  insn = emit_insn (insn);
>> +  insn = emit_insn (copy_rtx (cfa));
>> }
>
> This seems unrelated?

Now that insn is of type rtx_insn *, gen_rtx_SET () result cannot be
assigned to it.  Let me rewrite this by creating an rtx temporary,
which should be more readable.