This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[committed] Fix alloc_comp_assign_1.f90 on mips64-linux-gnu


This patch fixes an ICE in gfortran.dg/alloc_comp_assign_1.f90
for mips64-linux-gnu.  ivopts creates:

   MEM[index: D.2049_7239, offset: 24B]

This expands to (plus (reg: ...) (const_int 24)), which is indeed
a legitimate target address.  However, DOM later optimises this to:

   MEM[index: &y[0], offset: 24B]

which expands (plus (symbol_ref "y") (const_int 24)).  Note that
there's no (const ...) wrapper here.  Field-wise, the canonical
form would have been:

   MEM[symbol: y, offset: 24B]

-- which would have generated the missing (const ...) wrapper --
but that isn't a legitimate address on this target anyway.

IMO, this is a bit of flaw in tree optimisers.  The code that
creates TARGET_MEM_REFs is careful to only create references
that are legitimate target addresses, and makes sure that they
all have the canonical form described in tree.def:

--------------------------------------------------------------------------
/* Low-level memory addressing.  Operands are SYMBOL (static or global
   variable), BASE (register), INDEX (register), STEP (integer constant),
   OFFSET (integer constant).  Corresponding address is
   SYMBOL + BASE + STEP * INDEX + OFFSET.  Only variations and values valid on
   the target are allowed.
--------------------------------------------------------------------------

However, as seen here, later passes can replace the registers with
something else.

maybe_fold_tmr does try to fold integer base and index constants into the
offset, but if that new form is not a legitimate target address, it just
returns NULL.  fold_stmt will then keep the original form, with the
constant still in the base or index.  (maybe_fold_tmr doesn't try to
move symbolic constants to the "symbol" field, and would still have
to back off if the result wasn't legitimate anyway.)  Thus after creation,
it seems there's no particular rule about what the base and index fields
of a TARGET_MEM_REF hold, or whether the expression really is a target
address.

However -- getting to the point at last -- get_addr_rtx seems to assume
that the rules quoted above still hold, and uses gen_rtx_PLUS, etc.
directly.  So the function can create all sorts of weird rtl, such as
(plus (const_int ...) (const_int ...)), and a non-CONST
(plus (symbol_ref ...) (const_int ...)).  I wonder if it should be
using simplify_gen_binary when creating the final rtl (step_p and
offset_p are null then).  This would fix both of the examples
just cited.

But as things stand, this sort of address can reach the backend,
and the MIPS port wasn't robust against the non-CONST
(plus (symbol_ref ...) (const_int ...)).  We would end up creating:

    (lo_sum (reg ...)
            (plus (symbol_ref ...) (const_int ...)))

which isn't legitimate.

Fixed by the patch below.  Tested on mips64-linux-gnu (all 3 ABIs).
Applied to trunk and 4.2.

Richard


gcc/
	* config/mips/mips.c (mips_split_const): Don't accept bare PLUS
	expressions.

Index: gcc/config/mips/mips.c
===================================================================
--- gcc/config/mips/mips.c	(revision 117929)
+++ gcc/config/mips/mips.c	(working copy)
@@ -1282,12 +1282,13 @@ mips_split_const (rtx x, rtx *base, HOST
   *offset = 0;
 
   if (GET_CODE (x) == CONST)
-    x = XEXP (x, 0);
-
-  if (GET_CODE (x) == PLUS && GET_CODE (XEXP (x, 1)) == CONST_INT)
     {
-      *offset += INTVAL (XEXP (x, 1));
       x = XEXP (x, 0);
+      if (GET_CODE (x) == PLUS && GET_CODE (XEXP (x, 1)) == CONST_INT)
+	{
+	  *offset += INTVAL (XEXP (x, 1));
+	  x = XEXP (x, 0);
+	}
     }
   *base = x;
 }


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]