[PATCH] Fix find_reloads bugs w.r.t optional address operand reloads

["Ulrich Weigand" <Ulrich dot Weigand at de dot ibm dot com>]

Hello,

this fixes two reload bugs that caused an ICE on s390.  For an initial
discussion of the bugs see the thread:
  http://gcc.gnu.org/ml/gcc-patches/2002-11/msg01371.html

As pointed out by Joern Rennecke, the test case triggered a combination
of two bugs in reload.  This patch fixes both of them (either bug, when
fixed alone, lets the test case pass).

The first problem is that in some cases, there can be optional reloads
generated for address operands *after* the initial find_reloads_address
pass.  In those cases, reload would use the operand mode specified in
the insn pattern as reload mode.  This is incorrect for address operands,
where the operand mode has a different meaning.  I had already fixed
that case for the EXTRA_ADDRESS_OPERAND constraints, but it can also
occur for the regular 'p' constraint.  This patch uses the original
operand mode in all cases as the reload mode for address operands.

The second problem is that in most of those cases, it was actually
incorrect to generate the optional reload, because we in fact had
already pushed a mandatory reload from inside find_reloads_address
for the whole operand.  Joern Rennecke suggested to detect this
situation using find_replacement.  However, this does not work as
find_replacement assumes the actual reload registers have already
been filled in, which is not the case during find_reloads.  Thus,
I've added a new routine have_replacement_p, which simply checks
whether there is any replacement scheduled at a specified location.
For every operand where this holds true, no optional reloads are
generated.

Even this new function can only work on the final pass through
find_reloads, because the initial passes do not remember the
replacement locations at all.  However, as far as I can see,
the initial passes should not need to care about optional
reloads in the first place; the only thing calculate_needs_all_insn
does with optional reloads is to ignore them.  Thus, I've changed
find_reloads to simply not generate optional reloads at all
unless the REPLACE parameter is true.

Bootstrapped/regtested on s390-ibm-linux and s390x-ibm-linux,
both on mainline and the 3.3 branch.

OK to commit to mainline and 3.3?



ChangeLog:

gcc/
      * reload.c (find_reloads): Do not use the mode specified in the insn
      pattern as reload mode for address operands.  Do not generate optional
      reloads for operands where a mandatory reload was already pushed.
      Generate optional reloads only in the final pass though find_reloads.
      (have_replacement_p): New function.

gcc/testsuite/
      * gcc.dg/20030129-1.c: New test.


Index: gcc/reload.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/reload.c,v
retrieving revision 1.199.2.1
diff -c -p -r1.199.2.1 reload.c
*** gcc/reload.c  25 Jan 2003 23:59:25 -0000    1.199.2.1
--- gcc/reload.c  27 Jan 2003 17:31:23 -0000
*************** static void find_reloads_address_part PA
*** 272,277 ****
--- 272,278 ----
  static rtx find_reloads_subreg_address PARAMS ((rtx, int, int,
                                    enum reload_type, int, rtx));
  static void copy_replacements_1 PARAMS ((rtx *, rtx *, int));
+ static bool have_replacement_p    PARAMS ((rtx *));
  static int find_inc_amount  PARAMS ((rtx, rtx));

  #ifdef HAVE_SECONDARY_RELOADS
*************** find_reloads (insn, replace, ind_levels,
*** 2685,2690 ****
--- 2686,2695 ----

        recog_data.operand[i] = *recog_data.operand_loc[i];
        substed_operand[i] = recog_data.operand[i];
+
+       /* Address operands are reloaded in their existing mode,
+          no matter what is specified in the machine description.  */
+       operand_mode[i] = GET_MODE (recog_data.operand[i]);
      }
        else if (code == MEM)
      {
*************** find_reloads (insn, replace, ind_levels,
*** 3284,3293 ****
                     the address into a base register.  */
                  this_alternative[i] = (int) MODE_BASE_REG_CLASS (VOIDmode);
                  badop = 0;
-
-                 /* Address constraints are reloaded in Pmode, no matter
-                    what mode is given in the machine description.  */
-                 operand_mode[i] = Pmode;
                  break;
                  }

--- 3289,3294 ----
*************** find_reloads (insn, replace, ind_levels,
*** 3855,3863 ****
          return 0;
        }
        }
      else if (goal_alternative_matched[i] < 0
!          && goal_alternative_matches[i] < 0
!          && optimize)
        {
      /* For each non-matching operand that's a MEM or a pseudo-register
         that didn't get a hard register, make an optional reload.
--- 3856,3875 ----
          return 0;
        }
        }
+
+     /* Generate optional reloads only when optimizing, and only
+        on the last pass through reload.  Also, make sure we do not
+        make an optional reload where we already have a mandatory
+        one; this can happen in the case of address operands.
+
+        To check for mandatory reloads, we use have_replacement_p.
+        Note that this works only on the last pass through reload.  */
+     else if (!optimize || !replace
+          || have_replacement_p (recog_data.operand_loc[i]))
+       ; /* Do nothing.  */
+
      else if (goal_alternative_matched[i] < 0
!          && goal_alternative_matches[i] < 0)
        {
      /* For each non-matching operand that's a MEM or a pseudo-register
         that didn't get a hard register, make an optional reload.
*************** find_reloads (insn, replace, ind_levels,
*** 3907,3917 ****
         reload, check if this is actually a pseudo register reference;
         we then need to emit a USE and/or a CLOBBER so that reload
         inheritance will do the right thing.  */
!     else if (replace
!            && (GET_CODE (operand) == MEM
!                || (GET_CODE (operand) == REG
!                  && REGNO (operand) >= FIRST_PSEUDO_REGISTER
!                  && reg_renumber [REGNO (operand)] < 0)))
        {
          operand = *recog_data.operand_loc[i];

--- 3919,3928 ----
         reload, check if this is actually a pseudo register reference;
         we then need to emit a USE and/or a CLOBBER so that reload
         inheritance will do the right thing.  */
!     else if (GET_CODE (operand) == MEM
!            || (GET_CODE (operand) == REG
!                && REGNO (operand) >= FIRST_PSEUDO_REGISTER
!                && reg_renumber [REGNO (operand)] < 0))
        {
          operand = *recog_data.operand_loc[i];

*************** find_reloads (insn, replace, ind_levels,
*** 3934,3941 ****
           && goal_alternative_win[goal_alternative_matches[i]]
           && modified[i] == RELOAD_READ
           && modified[goal_alternative_matches[i]] == RELOAD_WRITE
!          && ! no_input_reloads && ! no_output_reloads
!          && optimize)
        {
      /* Similarly, make an optional reload for a pair of matching
         objects that are in MEM or a pseudo that didn't get a hard reg.  */
--- 3945,3951 ----
           && goal_alternative_win[goal_alternative_matches[i]]
           && modified[i] == RELOAD_READ
           && modified[goal_alternative_matches[i]] == RELOAD_WRITE
!          && ! no_input_reloads && ! no_output_reloads)
        {
      /* Similarly, make an optional reload for a pair of matching
         objects that are in MEM or a pseudo that didn't get a hard reg.  */
*************** find_replacement (loc)
*** 6107,6112 ****
--- 6117,6137 ----
      }

    return *loc;
+ }
+
+ /* Return true if some replacement was scheduled at LOC.  */
+
+ static bool
+ have_replacement_p (loc)
+      rtx *loc;
+ {
+   struct replacement *r;
+
+   for (r = &replacements[0]; r < &replacements[n_replacements]; r++)
+     if (r->where == loc)
+       return true;
+
+   return false;
  }

  /* Return nonzero if register in range [REGNO, ENDREGNO)
*** /dev/null     Thu Sep 19 11:33:10 2002
--- gcc/testsuite/gcc.dg/20030129-1.c     Wed Jan 29 14:52:45 2003
***************
*** 0 ****
--- 1,37 ----
+ /* This used to ICE due to a reload bug on s390*.  */
+
+ /* { dg-do compile { target s390*-*-* } } */
+ /* { dg-options "-O2" } */
+
+ int f (unsigned int);
+ void g (void *);
+
+ void test (void *p, void *dummy)
+ {
+   unsigned int flags = 0;
+
+   if (dummy)
+     g (dummy);
+
+   if (p)
+     flags |= 0x80000000;
+
+   asm volatile ("" : : : "1", "2", "3", "4", "5", "6", "7", "8", "9", "10", "11", "12");
+
+   if (dummy)
+     g (dummy);
+
+   if (p)
+     {
+       flags |= 0x20000000|0x80000000;
+
+       if (!f (0))
+         flags &= ~0x80000000;
+     }
+
+   f (flags);
+
+   if (dummy)
+     g (dummy);
+ }
+

Mit freundlichen Gruessen / Best Regards

Ulrich Weigand

--
  Dr. Ulrich Weigand
  Linux for S/390 Design & Development
  IBM Deutschland Entwicklung GmbH, Schoenaicher Str. 220, 71032 Boeblingen
  Phone: +49-7031/16-3727   ---   Email: Ulrich.Weigand@de.ibm.com