User account creation filtered due to spam.

Bug 36090 - [4.3/4.4 Regression] ppc64 cacoshl miscompilation
Summary: [4.3/4.4 Regression] ppc64 cacoshl miscompilation
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 4.3.1
: P1 normal
Target Milestone: 4.3.1
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on: 36182
Blocks:
  Show dependency treegraph
 
Reported: 2008-04-30 20:15 UTC by Jakub Jelinek
Modified: 2008-05-18 20:24 UTC (History)
8 users (show)

See Also:
Host:
Target: powerpc64-linux
Build:
Known to work:
Known to fail: 4.3.0
Last reconfirmed: 2008-04-30 21:17:51


Attachments
gcc44-pr36090.patch (1.80 KB, patch)
2008-05-02 09:46 UTC, Jakub Jelinek
Details | Diff
gcc44-pr36090.patch (847 bytes, patch)
2008-05-04 19:47 UTC, Jakub Jelinek
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jakub Jelinek 2008-04-30 20:15:07 UTC
extern void abort (void);

long double __attribute__ ((noinline)) foo (long double x)
{
  return __builtin_signbit (x) ? 3.1415926535897932384626433832795029L : 0.0;
}

int
main (void)
{
  if (foo (-1.0L) != 3.1415926535897932384626433832795029L)
    abort ();
  return 0;
}

is miscompiled with -O2 -m64 -mlong-double-128.
Both doubles of the long double return value from foo as the high double
of PI.
The assembly loads this long double value into 0/11 register pair using:
        ld 11,.LC1@toc(2)
        ld 0,.LC1@toc(2)
In RTL there is actually the +8 for the first insn present, but with double negation:
(insn:TI 40 20 39 t.c:5 (set (reg:DI 11 11 [orig:132 D.1237+8 ] [132])
        (mem/u/c/i:DI (plus:DI (reg:DI 2 2)
                (const:DI (minus:DI (symbol_ref/u:DI ("*.LC1") [flags 0x2])
                        (const:DI (plus:DI (symbol_ref:DI ("*.LCTOC1"))
                                (const_int -8 [0xfffffffffffffff8])))))) [2 S8 A64])) 334 {*movdi_internal64} (nil))

(insn 39 40 23 t.c:5 (set (reg:DI 0 0 [orig:131 D.1237 ] [131])
        (mem/u/c/i:DI (plus:DI (reg:DI 2 2)
                (const:DI (minus:DI (symbol_ref/u:DI ("*.LC1") [flags 0x2])
                        (symbol_ref:DI ("*.LCTOC1"))))) [2 S8 A128])) 334 {*movdi_internal64} (expr_list:REG_DEAD (reg:DI 2 2)
        (nil)))
So, either the output routines need to be fixed to handle even this weirdo addressing correctly and output 8+.LC1@toc(2), or the pass that generate this needs to be changed to generate something better.
Comment 1 Jakub Jelinek 2008-04-30 20:25:33 UTC
This wierdo addressing was created by fwprop2, which replaced:
In insn 40, replacing
 (mem/u/c/i:DI (plus:DI (reg/f:DI 129)
            (const_int 8 [0x8])) [2 S8 A64])
 with (mem/u/c/i:DI (plus:DI (reg:DI 2 2)
            (const:DI (minus:DI (symbol_ref/u:DI ("*.LC1") [flags 0x2])
                    (const:DI (plus:DI (symbol_ref:DI ("*.LCTOC1"))
                            (const_int -8 [0xfffffffffffffff8])))))) [2 S8 A64])
where
(insn 21 20 39 4 t.c:5 (set (reg/f:DI 129)
        (plus:DI (reg:DI 2 2)
            (const:DI (minus:DI (symbol_ref/u:DI ("*.LC1") [flags 0x2])
                    (symbol_ref:DI ("*.LCTOC1")))))) 334 {*movdi_internal64} (expr_list:REG_DEAD (reg:DI 2 2)
        (expr_list:REG_EQUAL (symbol_ref/u:DI ("*.LC1") [flags 0x2])
            (nil))))
Comment 2 Jakub Jelinek 2008-04-30 20:38:45 UTC
constant_pool_expr_p and therefore legitimate_constant_pool_address_p and
rs6000_legitimate_address too say this is ok, so fwprop2 isn't doing anything wrong.
Comment 3 David Edelsohn 2008-04-30 21:17:51 UTC
<jakub> Rhyolite: in PR36090, looks like print_operand_address completely ignores the other operand of MINUS, probably assumes it has to be .LCTOC1
Comment 4 David Edelsohn 2008-05-01 02:32:45 UTC
Is

        ld 11,8+.LC1@toc(2)

even valid assembly?  Is that a valid relocation?

I suspect that constant_pool_expr_1() needs to be changed so that it does not allow anything to be added to the toc_label_name.
Comment 5 David Edelsohn 2008-05-01 02:40:54 UTC
Maybe something like the following patch (untested)?

Index: rs6000.c
===================================================================
*** rs6000.c    (revision 132964)
--- rs6000.c    (working copy)
*************** constant_pool_expr_1 (rtx op, int *have_
*** 3358,3364 ****
        return 0;
      case PLUS:
      case MINUS:
!       return (constant_pool_expr_1 (XEXP (op, 0), have_sym, have_toc)
              && constant_pool_expr_1 (XEXP (op, 1), have_sym, have_toc));
      case CONST:
        return constant_pool_expr_1 (XEXP (op, 0), have_sym, have_toc);
--- 3358,3365 ----
        return 0;
      case PLUS:
      case MINUS:
!       return (! *have_toc
!             && constant_pool_expr_1 (XEXP (op, 0), have_sym, have_toc)
              && constant_pool_expr_1 (XEXP (op, 1), have_sym, have_toc));
      case CONST:
        return constant_pool_expr_1 (XEXP (op, 0), have_sym, have_toc);
Comment 6 Jakub Jelinek 2008-05-01 07:13:39 UTC
It certainly assembles with gas and after adding that 8+ the testcase works.
Comment 7 David Edelsohn 2008-05-01 15:08:05 UTC
The TOC path is used for both PPC64 Linux and AIX, so it needs to be acceptable for both systems.

        ld 11,8+LC..1(2)

is valid AIX assembler, so we can pursue that solution.
Comment 8 David Edelsohn 2008-05-01 15:46:14 UTC
We can print the offset with something like the following patch:

Index: rs6000.c
===================================================================
*** rs6000.c    (revision 134851)
--- rs6000.c    (working copy)
*************** print_operand_address (FILE *file, rtx x
*** 12346,12351 ****
--- 12346,12355 ----
            contains_minus = XEXP (contains_minus, 0);

          minus = XEXP (contains_minus, 0);
+         if (GET_CODE (XEXP (minus, 1)) == CONST
+             && (GET_CODE (XEXP (XEXP (minus, 1), 0)) == PLUS))
+           fprintf (file, HOST_WIDE_INT_PRINT_DEC"+",
+                    -INTVAL (XEXP (XEXP (XEXP (minus, 1), 0), 1)));
          symref = XEXP (minus, 0);
          XEXP (contains_minus, 0) = symref;
          if (TARGET_ELF)
Comment 9 Jakub Jelinek 2008-05-01 17:15:56 UTC
Yeah, this would fix this testcase.  But I'd say constant_pool_expr_1 or callers
need to be adjusted, so that they only accept what print_operand_address can handle.
BTW, for:
long double foo (void)
{
  return 2.152121340829751328947138941734L;
}
gcc generates:
(insn 24 23 16 x.c:4 (set (reg:DF 34 2 [orig:33 <result>+8 ] [33])
        (mem/u/c/i:DF (plus:DI (reg:DI 2 2)
                (const:DI (plus:DI (minus:DI (symbol_ref/u:DI ("*.LC0") [flags 0x2])
                            (symbol_ref:DI ("*.LCTOC1")))
                        (const_int 8 [0x8])))) [2 S8 A64])) 322 {*movdf_hardfloat64} (expr_list:REG_DEAD (reg:DI 2 2)
        (nil)))
and emits
lfd 2,.LC0@toc+8(2)
Comment 10 David Edelsohn 2008-05-01 17:33:19 UTC
In your second example, the RTL is in the canonical form that I would have expected and already is handled:

(mem/u/c/i:DF (plus:DI (reg:DI 2 2)
        (const:DI (plus:DI (minus:DI (symbol_ref/u:DI ("*.LC0") [flags 0x2])
                    (symbol_ref:DI ("*.LCTOC1")))
                (const_int 8 [0x8])))) [2 S8 A64])

The other solution is to ensure the propagated value produces canonical RTL.
Comment 11 Jakub Jelinek 2008-05-02 09:34:53 UTC
The unexpected address is created by simplify_plus_minus on
(plus:DI (reg:DI 2 2)
    (const:DI (minus:DI (symbol_ref/u:DI ("*.LC1") [flags 0x2])
            (symbol_ref:DI ("*.LCTOC1")))))
and
(const_int 8 [0x8])
where the .LCTOC1 and +8 pair is simplified together.  Unfortunately, if we refuse it in rs6000_legitimate_address, fwprop doesn't try harder (LEGITIMIZE_ADDRESS/memory_address isn't called) and nothing afterwards propagates it either, so we end up with:
la 9,.LC1@toc(2)
ld 0,8(9)
instead of
ld 0,.LC1@toc+8(2)
So I'm afraid we want to accept it.
Comment 12 Jakub Jelinek 2008-05-02 09:46:50 UTC
Created attachment 15561 [details]
gcc44-pr36090.patch

Untested patch that 1) tightens the checking in legitimate_constant_pool_address_p - previously it could accept e.g. two symrefs, two tocrefs or say MINUS (tocref, symref) etc. 2) should be able to output everything that is accepted by that routine.  All I've tested so far is that it fixes this testcase.
Comment 13 David Edelsohn 2008-05-02 12:41:25 UTC
fwprop cannot be modified to produce the preferred RTL with the symbol_refs on the inside and the constant on the outside instead of teaching another part of the compiler how to print this peculiar construct?
Comment 14 David Edelsohn 2008-05-02 15:10:25 UTC
The problematic transformation in simplify_plus_minus() is:

  /* We suppressed creation of trivial CONST expressions in the
     combination loop to avoid recursion.  Create one manually now.
     The combination loop should have ensured that there is exactly
     one CONST_INT, and the sort will have ensured that it is last
     in the array and that any other constant will be next-to-last.  */

  if (n_ops > 1
      && GET_CODE (ops[n_ops - 1].op) == CONST_INT
      && CONSTANT_P (ops[n_ops - 2].op))
    {
      rtx value = ops[n_ops - 1].op;
      if (ops[n_ops - 1].neg ^ ops[n_ops - 2].neg)
        value = neg_const_int (mode, value);
      ops[n_ops - 2].op = plus_constant (ops[n_ops - 2].op, INTVAL (value));
      n_ops--;
    }

If that transformation is avoided, the preferred RTL is generated.
Comment 15 David Edelsohn 2008-05-02 15:43:36 UTC
This patch groups RTX_CONST_OBJ before CONST_INT.

Index: simplify-rtx.c
===================================================================
*** simplify-rtx.c      (revision 134851)
--- simplify-rtx.c      (working copy)
*************** simplify_plus_minus (enum rtx_code code,
*** 3676,3682 ****
  
    if (n_ops > 1
        && GET_CODE (ops[n_ops - 1].op) == CONST_INT
!       && CONSTANT_P (ops[n_ops - 2].op))
      {
        rtx value = ops[n_ops - 1].op;
        if (ops[n_ops - 1].neg ^ ops[n_ops - 2].neg)
--- 3676,3684 ----
  
    if (n_ops > 1
        && GET_CODE (ops[n_ops - 1].op) == CONST_INT
!       && CONSTANT_P (ops[n_ops - 2].op)
!       && (n_ops < 3
!         || !CONSTANT_P (ops[n_ops - 3].op)))
      {
        rtx value = ops[n_ops - 1].op;
        if (ops[n_ops - 1].neg ^ ops[n_ops - 2].neg)
Comment 16 David Edelsohn 2008-05-02 15:45:43 UTC
Copying Bonzini for him to comment on the precedence and canonicalization issue.
Comment 17 Paolo Bonzini 2008-05-02 16:29:57 UTC
I wonder if your patch would be a problem because it sometimes removes a CONST wrapping.  It could also be possible to precede the CONST_INT optimization with another test for two adjacent CONSTANT_P:

  if (GET_CODE (ops[n_ops - 1]) == CONST_INT)
    i = n_ops - 2;
  else
    i = n_ops - 1;

  if (i >= 1
      && ops[i].neg
      && !ops[i - 1].neg
      && CONSTANT_P (ops[i].op)
      && GET_CODE (ops[i].op) == GET_CODE (ops[i - 1].op))
    {
      ops[i - 1].op = gen_rtx_MINUS (mode, ops[i - 1].op, ops[i].op);
      ops[i - 1].op = gen_rtx_CONST (mode, ops[i - 1].op);
      if (i < n_ops - 1)
        ops[i] = op[i + 1];
      n_ops--;
    }

Keeping together a (minus (symbol_ref) (symbol_ref)), which could also be a (minus (label_ref (label_ref)), seems like a good idea.

Does this work?
Comment 18 David Edelsohn 2008-05-02 17:36:33 UTC
Yes, the patch works, modulo typos.

  if (GET_CODE (ops[n_ops - 1].op) == CONST_INT)
    i = n_ops - 2;
  else
    i = n_ops - 1;

  if (i >= 1
      && ops[i].neg
      && !ops[i - 1].neg
      && CONSTANT_P (ops[i].op)
      && GET_CODE (ops[i].op) == GET_CODE (ops[i - 1].op))
    {
      ops[i - 1].op = gen_rtx_MINUS (mode, ops[i - 1].op, ops[i].op);
      ops[i - 1].op = gen_rtx_CONST (mode, ops[i - 1].op);
      if (i < n_ops - 1)
        ops[i] = ops[i + 1];
      n_ops--;
    }

plus_constant strips the inner CONST and the earlier version also wrapped everything in CONST.

Does it make sense to group any two RTX_CONST_OBJ together of not the same type?
Comment 19 Paolo Bonzini 2008-05-02 21:08:05 UTC
Subject: Re:  [4.3/4.4 Regression] ppc64 cacoshl miscompilation

> Yes, the patch works, modulo typos.

It was not tested indeed.

> Does it make sense to group any two RTX_CONST_OBJ together of not the same
> type?

I don't know, but replacing the second test with CONSTANT_P should also 
be safe.
Comment 20 David Edelsohn 2008-05-02 21:23:40 UTC
How do we proceed?  Your initial patch is fine with me.
Comment 21 Paolo Bonzini 2008-05-03 07:13:05 UTC
Subject: Re:  [4.3/4.4 Regression] ppc64 cacoshl miscompilation


> How do we proceed?  Your initial patch is fine with me.

Whose?  I can foster-parent yours too, and bootstrap/regtest it on 
i686-pc-linux-gnu.

Paolo

Comment 22 David Edelsohn 2008-05-03 13:21:51 UTC
Your patch from comment #17 (with typos fixed in comment #18).
Comment 23 Paolo Bonzini 2008-05-03 13:25:35 UTC
Subject: Re:  [4.3/4.4 Regression] ppc64 cacoshl miscompilation

dje at gcc dot gnu dot org wrote:
> ------- Comment #22 from dje at gcc dot gnu dot org  2008-05-03 13:21 -------
> Your patch from comment #17 (with typos fixed in comment #18).

Have you bootstrapped/regtested it?  I can alternatively do it but only 
on i686-pc-linux-gnu.

Paolo
Comment 24 David Edelsohn 2008-05-03 14:20:53 UTC
I am testing with the patch.  I wanted to make sure that there was agreement on how to modify simplify_plus_minus before going too far.  Also, I wanted to give you the opportunity to take the lead on your patch.  I can shepherd the patch, if you prefer.
Comment 25 Paolo Bonzini 2008-05-03 15:47:57 UTC
Subject: Re:  [4.3/4.4 Regression] ppc64 cacoshl miscompilation

dje at gcc dot gnu dot org wrote:
> ------- Comment #24 from dje at gcc dot gnu dot org  2008-05-03 14:20 -------
> I am testing with the patch.  I wanted to make sure that there was agreement on
> how to modify simplify_plus_minus before going too far.  Also, I wanted to give
> you the opportunity to take the lead on your patch.  I can shepherd the patch,
> if you prefer.

I'd have tested testing on >1 platform, so I'll test anyway for 
i686-pc-linux-gnu -- but no sooner than Monday.  Since this is for 
4.3.1, it's probably best that you post it as soon as teseting is finished.

Paolo
Comment 26 Jakub Jelinek 2008-05-04 19:47:23 UTC
Created attachment 15576 [details]
gcc44-pr36090.patch

Patch I've bootstrapped on 4.3 branch on {i386,x86_64,ppc,ppc64}-linux, no regressions.

Note that while this fixes this regression, IMNSHO rs6000_legitimate_address check needs to be tightened up anyway, to match what the output routine actually accepts (with current print_operand_address code the first MINUS when going through XEXP (x, 0) must be symbol - .LCTOC*).
Comment 27 David Edelsohn 2008-05-04 20:08:52 UTC
I was planning to add asserts in print_operand_address ensuring that the operand was the TOC SYMBOL_REF.
Comment 28 David Edelsohn 2008-05-08 16:36:44 UTC
Subject: Bug 36090

Author: dje
Date: Thu May  8 16:35:56 2008
New Revision: 135086

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=135086
Log:
2008-05-08  Paolo Bonzini  <bonzini@gnu.org>

        PR target/36090
        * simplify-rtx.c (simplify_plus_minus): Create CONST of
        similar RTX_CONST_OBJ before CONST_INT.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/simplify-rtx.c

Comment 29 David Edelsohn 2008-05-08 16:41:02 UTC
Subject: Bug 36090

Author: dje
Date: Thu May  8 16:40:17 2008
New Revision: 135087

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=135087
Log:
2008-05-08  Paolo Bonzini  <bonzini@gnu.org>

        PR target/36090
        * simplify-rtx.c (simplify_plus_minus): Create CONST of
        similar RTX_CONST_OBJ before CONST_INT.

Modified:
    branches/gcc-4_3-branch/gcc/ChangeLog
    branches/gcc-4_3-branch/gcc/simplify-rtx.c

Comment 30 littlestar 2008-05-09 05:36:42 UTC
fixed?
Comment 31 Jakub Jelinek 2008-05-09 14:18:25 UTC
Alternate patch to #c12 that doesn't touch at all rs6000_legitimate_address,
just changes print_operand_address:

--- gcc/config/rs6000/rs6000.c.jj       2008-04-24 18:30:39.000000000 +0200
+++ gcc/config/rs6000/rs6000.c  2008-05-09 16:10:24.000000000 +0200
@@ -723,6 +723,7 @@ static unsigned rs6000_hash_constant (rt
 static unsigned toc_hash_function (const void *);
 static int toc_hash_eq (const void *, const void *);
 static int constant_pool_expr_1 (rtx, int *, int *);
+static int find_constant_pool_expr (rtx *, rtx **, rtx **);
 static bool constant_pool_expr_p (rtx);
 static bool legitimate_small_data_p (enum machine_mode, rtx);
 static bool legitimate_lo_sum_address_p (enum machine_mode, rtx, int);
@@ -3335,6 +3336,44 @@ constant_pool_expr_1 (rtx op, int *have_
     }
 }
 
+static int
+find_constant_pool_expr (rtx *op, rtx **sym, rtx **toc)
+{
+  switch (GET_CODE (*op))
+    {
+    case SYMBOL_REF:
+      if (RS6000_SYMBOL_REF_TLS_P (*op))
+       return 0;
+      else if (CONSTANT_POOL_ADDRESS_P (*op))
+       {
+         if (ASM_OUTPUT_SPECIAL_POOL_ENTRY_P (get_pool_constant (*op), Pmode))
+           {
+             *sym = op;
+             return 1;
+           }
+         else
+           return 0;
+       }
+      else if (! strcmp (XSTR (*op, 0), toc_label_name))
+       {
+         *toc = op;
+         return 1;
+       }
+      else
+       return 0;
+    case PLUS:
+    case MINUS:
+      return (find_constant_pool_expr (&XEXP (*op, 0), sym, toc)
+             && find_constant_pool_expr (&XEXP (*op, 1), sym, toc));
+    case CONST:
+      return find_constant_pool_expr (&XEXP (*op, 0), sym, toc);
+    case CONST_INT:
+      return 1;
+    default:
+      return 0;
+    }
+}
+
 static bool
 constant_pool_expr_p (rtx op)
 {
@@ -12268,32 +12307,36 @@ print_operand_address (FILE *file, rtx x
     {
       if (TARGET_AIX && (!TARGET_ELF || !TARGET_MINIMAL_TOC))
        {
-         rtx contains_minus = XEXP (x, 1);
-         rtx minus, symref;
+         rtx *symref = NULL, *tocref = NULL, toc = NULL, addr;
          const char *name;
 
-         /* Find the (minus (sym) (toc)) buried in X, and temporarily
-            turn it into (sym) for output_addr_const.  */
-         while (GET_CODE (XEXP (contains_minus, 0)) != MINUS)
-           contains_minus = XEXP (contains_minus, 0);
-
-         minus = XEXP (contains_minus, 0);
-         symref = XEXP (minus, 0);
-         XEXP (contains_minus, 0) = symref;
+         find_constant_pool_expr (&XEXP (x, 1), &symref, &tocref);
+         if (tocref)
+           {
+             toc = *tocref;
+             *tocref = const0_rtx;
+           }
          if (TARGET_ELF)
            {
              char *newname;
 
-             name = XSTR (symref, 0);
+             name = XSTR (*symref, 0);
              newname = alloca (strlen (name) + sizeof ("@toc"));
              strcpy (newname, name);
              strcat (newname, "@toc");
-             XSTR (symref, 0) = newname;
+             XSTR (*symref, 0) = newname;
            }
-         output_addr_const (file, XEXP (x, 1));
+         addr = XEXP (x, 1);
+         if (GET_CODE (addr) == CONST)
+           addr = XEXP (addr, 0);
+         addr = simplify_rtx (addr);
+         if (!addr)
+           addr = XEXP (x, 1);
+         output_addr_const (file, addr);
          if (TARGET_ELF)
-           XSTR (symref, 0) = name;
-         XEXP (contains_minus, 0) = minus;
+           XSTR (*symref, 0) = name;
+         if (tocref)
+           *tocref = toc;
        }
       else
        output_addr_const (file, XEXP (x, 1));


The differences from #c12: 1) it doesn't reuse the same routine for finding
the addresses.  The disadvantage of this is that whenever making changes to
constant_pool_expr_1 the other should be adjusted too.
2) it will silently miscompile things if more than one sym or more than one toc
is in the address (unlikely to be ever generated, true) 3) similarly, it doesn't
check that toc is subtracted from sym, so in some pathological case we could have
.LC1 + .LCTOC1 or -.LC1 - .LCTOC1.  No testing done on the patch except for testing that it fixes the testcase.
Comment 32 Jakub Jelinek 2008-05-09 14:32:19 UTC
Actually find_constant_pool_expr can be simplified, given that constant_pool_expr_1 had to return 1 each time.

--- gcc/config/rs6000/rs6000.c.jj       2008-04-24 18:30:39.000000000 +0200
+++ gcc/config/rs6000/rs6000.c  2008-05-09 16:28:39.000000000 +0200
@@ -723,6 +723,7 @@ static unsigned rs6000_hash_constant (rt
 static unsigned toc_hash_function (const void *);
 static int toc_hash_eq (const void *, const void *);
 static int constant_pool_expr_1 (rtx, int *, int *);
+static void find_constant_pool_expr (rtx *, rtx **, rtx **);
 static bool constant_pool_expr_p (rtx);
 static bool legitimate_small_data_p (enum machine_mode, rtx);
 static bool legitimate_lo_sum_address_p (enum machine_mode, rtx, int);
@@ -3335,6 +3336,32 @@ constant_pool_expr_1 (rtx op, int *have_
     }
 }
 
+static void
+find_constant_pool_expr (rtx *op, rtx **sym, rtx **toc)
+{
+  switch (GET_CODE (*op))
+    {
+    case SYMBOL_REF:
+      if (CONSTANT_POOL_ADDRESS_P (*op))
+       *sym = op;
+      else
+       *toc = op;
+      break;
+    case PLUS:
+    case MINUS:
+      find_constant_pool_expr (&XEXP (*op, 0), sym, toc);
+      find_constant_pool_expr (&XEXP (*op, 1), sym, toc);
+      break;
+    case CONST:
+      find_constant_pool_expr (&XEXP (*op, 0), sym, toc);
+      break;
+    case CONST_INT:
+      break;
+    default:
+      gcc_unreachable ();
+    }
+}
+
 static bool
 constant_pool_expr_p (rtx op)
 {
@@ -12268,32 +12295,36 @@ print_operand_address (FILE *file, rtx x
     {
       if (TARGET_AIX && (!TARGET_ELF || !TARGET_MINIMAL_TOC))
        {
-         rtx contains_minus = XEXP (x, 1);
-         rtx minus, symref;
+         rtx *symref = NULL, *tocref = NULL, toc = NULL, addr;
          const char *name;
 
-         /* Find the (minus (sym) (toc)) buried in X, and temporarily
-            turn it into (sym) for output_addr_const.  */
-         while (GET_CODE (XEXP (contains_minus, 0)) != MINUS)
-           contains_minus = XEXP (contains_minus, 0);
-
-         minus = XEXP (contains_minus, 0);
-         symref = XEXP (minus, 0);
-         XEXP (contains_minus, 0) = symref;
+         find_constant_pool_expr (&XEXP (x, 1), &symref, &tocref);
+         if (tocref)
+           {
+             toc = *tocref;
+             *tocref = const0_rtx;
+           }
          if (TARGET_ELF)
            {
              char *newname;
 
-             name = XSTR (symref, 0);
+             name = XSTR (*symref, 0);
              newname = alloca (strlen (name) + sizeof ("@toc"));
              strcpy (newname, name);
              strcat (newname, "@toc");
-             XSTR (symref, 0) = newname;
+             XSTR (*symref, 0) = newname;
            }
-         output_addr_const (file, XEXP (x, 1));
+         addr = XEXP (x, 1);
+         if (GET_CODE (addr) == CONST)
+           addr = XEXP (addr, 0);
+         addr = simplify_rtx (addr);
+         if (!addr)
+           addr = XEXP (x, 1);
+         output_addr_const (file, addr);
          if (TARGET_ELF)
-           XSTR (symref, 0) = name;
-         XEXP (contains_minus, 0) = minus;
+           XSTR (*symref, 0) = name;
+         if (tocref)
+           *tocref = toc;
        }
       else
        output_addr_const (file, XEXP (x, 1));

Comment 33 Paolo Bonzini 2008-05-09 15:51:56 UTC
I now think that fixing it in the target is better.
Comment 34 David Edelsohn 2008-05-09 17:14:20 UTC
Subject: Bug 36090

Author: dje
Date: Fri May  9 17:13:30 2008
New Revision: 135118

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=135118
Log:
        PR target/36090
        * config/rs6000/rs6000.c (print_operand_address): If the TOC
        address RHS contains an offset, output it as well.

        PR target/36182
        Revert:
        2008-05-08  Paolo Bonzini  <bonzini@gnu.org>
        PR target/36090
        * simplify-rtx.c (simplify_plus_minus): Create CONST of
        similar RTX_CONST_OBJ before CONST_INT.

Modified:
    branches/gcc-4_3-branch/gcc/ChangeLog
    branches/gcc-4_3-branch/gcc/config/rs6000/rs6000.c
    branches/gcc-4_3-branch/gcc/simplify-rtx.c

Comment 35 Paolo Bonzini 2008-05-12 15:50:21 UTC
so you prefer to leave my fix (possibly amended in 4.4+)?
Comment 36 David Edelsohn 2008-05-12 16:32:33 UTC
Yes, I suggest keeping the simplify_plus_minus change with the CONST wrapper removed.
Comment 37 Paolo Bonzini 2008-05-12 16:48:00 UTC
Ok, I started a bootstrap of the obvious patch on i686-pc-linux-gnu.  But I think the obvious patch is not enough if we go this way.  The comments in simplify_plus_minus are already not up-to-date, and removing CONST is "interesting" enough that the comment should be updated with clear explanations (e.g. based on the info H-P gave in PR36182).  Would you please do so since you have better hold on this than I do?

Otherwise, as I said, I have absolutely no problem going for the target-only fix in 4.4+ too.
Comment 38 David Edelsohn 2008-05-12 16:51:55 UTC
I did update the comment when I applied the patch.

I will work on this more when I return from travel next week.
Comment 39 Paolo Bonzini 2008-05-12 16:54:58 UTC
Ah, only on 4.3 branch.  Ok, I'll go on from here.
Comment 40 Richard Biener 2008-05-16 19:44:10 UTC
What is the status of this bug on the 4.3 branch?
Comment 41 Jakub Jelinek 2008-05-18 18:31:31 UTC
The bug is fixed there (though with the safer rs6000 specific fix), just the testcase hasn't been committed.  I'll do that later tonight.
Comment 42 Jakub Jelinek 2008-05-18 20:19:48 UTC
Subject: Bug 36090

Author: jakub
Date: Sun May 18 20:19:03 2008
New Revision: 135508

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=135508
Log:
	PR target/36090
	* gcc.c-torture/execute/20080502-1.c: New test.

Added:
    trunk/gcc/testsuite/gcc.c-torture/execute/20080502-1.c
Modified:
    trunk/gcc/testsuite/ChangeLog

Comment 43 Jakub Jelinek 2008-05-18 20:20:51 UTC
Subject: Bug 36090

Author: jakub
Date: Sun May 18 20:19:55 2008
New Revision: 135509

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=135509
Log:
	PR target/36090
	* gcc.c-torture/execute/20080502-1.c: New test.

Added:
    branches/gcc-4_3-branch/gcc/testsuite/gcc.c-torture/execute/20080502-1.c
Modified:
    branches/gcc-4_3-branch/gcc/testsuite/ChangeLog

Comment 44 Jakub Jelinek 2008-05-18 20:24:21 UTC
The bug I've reported is fixed both on the trunk and on the branch, although
with different patches.