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]

Fix E500 wrong-code bug


This patch (an rs6000 back-end patch plus a middle-end assertion to help 
detect such bugs in future) fixes a wrong-code bug for E500.  (The 
original bug was observed as a libstdc++ test failure for a 4.2-based 
toolchain; that failure no longer appears with trunk, but as far as I can 
tell the underlying issue is still present.)

The problem was that

(insn 213 212 214 9 (set (mem/s/c:V2SI (plus:SI (reg/f:SI 115 virtual-stack-vars)
                (const_int 33592 [0x8338])) [0 S8 A128])
        (reg:V2SI 225)) -1 (nil)
    (nil))

in the .unshare dump turns into

(insn 213 354 214 9 (set (reg:V2SI 280)
        (reg:V2SI 225)) 1026 {*movv2si_internal} (nil)
    (nil))

in the .vregs dump (and reg 280 isn't used after being set there).  This 
is storing a structure value, whose address is passed as a parameter to 
the "copy" function, on the stack; the address still gets passed, but the 
structure hasn't been initialized, but with the more-reduced testcase the 
structure does get initialized and so the test passes.

The problem transformation above is done by 
instantiate_virtual_regs_in_insn.  We have a loop

  /* In the general case, we expect virtual registers to appear only in
     operands, and then only as either bare registers or inside memories.  */
  for (i = 0; i < recog_data.n_operands; ++i)
    {
      x = recog_data.operand[i];
      switch (GET_CODE (x))
        {
        case MEM:
          {
            rtx addr = XEXP (x, 0);
            bool changed = false;

            for_each_rtx (&addr, instantiate_virtual_regs_in_rtx, &changed);
            if (!changed)
              continue;

            start_sequence ();
            x = replace_equiv_address (x, addr);
            seq = get_insns ();
            end_sequence ();
            if (seq)
              emit_insn_before (seq, insn);
          }
          break;
[...]

      /* At this point, X contains the new value for the operand.
         Validate the new value vs the insn predicate.  Note that
         asm insns will have insn_code -1 here.  */
      if (!safe_insn_predicate (insn_code, i, x))
        {
          start_sequence ();
          x = force_reg (insn_data[insn_code].operand[i].mode, x);
          seq = get_insns ();
          end_sequence ();
          if (seq)
            emit_insn_before (seq, insn);
        }

and the new value of x for the MEM above fails to satisfy 
safe_insn_predicate, so force_reg generates the REG above.  This won't 
work for the output of the SET; presumably it's expected that only inputs 
should ever get to force_reg here.

The MEM should have been made valid by replace_equiv_address.  However, 
replace_equiv_address ends up calling memory_address to create a valid 
address, which calls LEGITIMIZE_ADDRESS - which changes the address and 
returns one which still isn't valid for V2SImode.

Hence the patch below.  I fixed rs6000_legitimize_address not to apply an 
inappropriate strategy to addresses for E500 modes that have more 
restricted offset ranges (and are handled further down the function) and 
added an assertion to memory_address that the resulting address is valid.

Tested with no regressions with cross to powerpc-none-linux-gnuspe; also 
bootstrapped with no regressions on i686-pc-linux-gnu to make sure the 
assertion doesn't fail there.  OK to commit?

2007-09-27  Joseph Myers  <joseph@codesourcery.com>

	* explow.c (memory_address): Assert that the generated address is
	valid.
	* config/rs6000/rs6000.c (rs6000_legitimize_address): Do not
	reduce offset by units of 0x10000 for SPE vector modes or modes
	used with E500 double instructions.

Index: gcc/explow.c
===================================================================
--- gcc/explow.c	(revision 128824)
+++ gcc/explow.c	(working copy)
@@ -497,6 +497,7 @@
 
  done:
 
+  gcc_assert (memory_address_p (mode, x));
   /* If we didn't change the address, we are done.  Otherwise, mark
      a reg as a pointer if we have REG or REG + CONST_INT.  */
   if (oldx == x)
Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 128824)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -3381,7 +3381,10 @@
   if (GET_CODE (x) == PLUS
       && GET_CODE (XEXP (x, 0)) == REG
       && GET_CODE (XEXP (x, 1)) == CONST_INT
-      && (unsigned HOST_WIDE_INT) (INTVAL (XEXP (x, 1)) + 0x8000) >= 0x10000)
+      && (unsigned HOST_WIDE_INT) (INTVAL (XEXP (x, 1)) + 0x8000) >= 0x10000
+      && !(SPE_VECTOR_MODE (mode)
+	   || (TARGET_E500_DOUBLE && (mode == DFmode || mode == TFmode
+				      || mode == DImode))))
     {
       HOST_WIDE_INT high_int, low_int;
       rtx sum;

-- 
Joseph S. Myers
joseph@codesourcery.com


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