[Bug target/36090] [4.3/4.4 Regression] ppc64 cacoshl miscompilation

[Bug target/36090] [4.3/4.4 Regression] ppc64 cacoshl miscompilation

[jakub at gcc dot gnu dot org]

-- 

jakub at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P1
   Target Milestone|---                         |4.3.1


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36090

[Bug target/36090] New: [4.3/4.4 Regression] ppc64 cacoshl miscompilation

[jakub at gcc dot gnu dot org]

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.


-- 
           Summary: [4.3/4.4 Regression] ppc64 cacoshl miscompilation
           Product: gcc
           Version: 4.3.1
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: jakub at gcc dot gnu dot org
GCC target triplet: powerpc64-linux


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36090

[Bug target/36090] [4.3/4.4 Regression] ppc64 cacoshl miscompilation

[jakub at gcc dot gnu dot org]

------- Comment #1 from jakub at gcc dot gnu dot org  2008-04-30 20:25 -------
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))))


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36090

[Bug target/36090] [4.3/4.4 Regression] ppc64 cacoshl miscompilation

[jakub at gcc dot gnu dot org]

------- Comment #2 from jakub at gcc dot gnu dot org  2008-04-30 20:38 -------
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.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36090

[Bug target/36090] [4.3/4.4 Regression] ppc64 cacoshl miscompilation

[dje at gcc dot gnu dot org]

------- Comment #3 from dje at gcc dot gnu dot org  2008-04-30 21:17 -------
<jakub> Rhyolite: in PR36090, looks like print_operand_address completely
ignores the other operand of MINUS, probably assumes it has to be .LCTOC1


-- 

dje at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
     Ever Confirmed|0                           |1
   Last reconfirmed|0000-00-00 00:00:00         |2008-04-30 21:17:51
               date|                            |


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36090

[Bug target/36090] [4.3/4.4 Regression] ppc64 cacoshl miscompilation

[dje at gcc dot gnu dot org]

------- Comment #4 from dje at gcc dot gnu dot org  2008-05-01 02:32 -------
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.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36090

[Bug target/36090] [4.3/4.4 Regression] ppc64 cacoshl miscompilation

[dje at gcc dot gnu dot org]

------- Comment #5 from dje at gcc dot gnu dot org  2008-05-01 02:40 -------
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);


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36090

[Bug target/36090] [4.3/4.4 Regression] ppc64 cacoshl miscompilation

[jakub at gcc dot gnu dot org]

------- Comment #6 from jakub at gcc dot gnu dot org  2008-05-01 07:13 -------
It certainly assembles with gas and after adding that 8+ the testcase works.


-- 

jakub at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |amodra at gcc dot gnu dot
                   |                            |org


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36090

[Bug target/36090] [4.3/4.4 Regression] ppc64 cacoshl miscompilation

[dje at gcc dot gnu dot org]

------- Comment #7 from dje at gcc dot gnu dot org  2008-05-01 15:08 -------
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.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36090

[Bug target/36090] [4.3/4.4 Regression] ppc64 cacoshl miscompilation

[dje at gcc dot gnu dot org]

------- Comment #8 from dje at gcc dot gnu dot org  2008-05-01 15:46 -------
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)


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36090

[Bug target/36090] [4.3/4.4 Regression] ppc64 cacoshl miscompilation

[jakub at gcc dot gnu dot org]

------- Comment #9 from jakub at gcc dot gnu dot org  2008-05-01 17:15 -------
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)


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36090

[Bug target/36090] [4.3/4.4 Regression] ppc64 cacoshl miscompilation

[dje at gcc dot gnu dot org]

------- Comment #10 from dje at gcc dot gnu dot org  2008-05-01 17:33 -------
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.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36090

[Bug target/36090] [4.3/4.4 Regression] ppc64 cacoshl miscompilation

[jakub at gcc dot gnu dot org]

------- Comment #11 from jakub at gcc dot gnu dot org  2008-05-02 09:34 -------
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.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36090

[Bug target/36090] [4.3/4.4 Regression] ppc64 cacoshl miscompilation

[jakub at gcc dot gnu dot org]

------- Comment #12 from jakub at gcc dot gnu dot org  2008-05-02 09:46 -------
Created an attachment (id=15561)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=15561&action=view)
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.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36090

[Bug target/36090] [4.3/4.4 Regression] ppc64 cacoshl miscompilation

[dje at gcc dot gnu dot org]

------- Comment #13 from dje at gcc dot gnu dot org  2008-05-02 12:41 -------
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?


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36090

[Bug target/36090] [4.3/4.4 Regression] ppc64 cacoshl miscompilation

[dje at gcc dot gnu dot org]

------- Comment #14 from dje at gcc dot gnu dot org  2008-05-02 15:10 -------
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.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36090

[Bug target/36090] [4.3/4.4 Regression] ppc64 cacoshl miscompilation

[dje at gcc dot gnu dot org]

------- Comment #15 from dje at gcc dot gnu dot org  2008-05-02 15:43 -------
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)


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36090

[Bug target/36090] [4.3/4.4 Regression] ppc64 cacoshl miscompilation

[dje at gcc dot gnu dot org]

------- Comment #16 from dje at gcc dot gnu dot org  2008-05-02 15:45 -------
Copying Bonzini for him to comment on the precedence and canonicalization
issue.


-- 

dje at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |bonzini at gnu dot org


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36090

[Bug target/36090] [4.3/4.4 Regression] ppc64 cacoshl miscompilation

[bonzini at gnu dot org]

------- Comment #17 from bonzini at gnu dot org  2008-05-02 16:29 -------
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?


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36090

[Bug target/36090] [4.3/4.4 Regression] ppc64 cacoshl miscompilation

[dje at gcc dot gnu dot org]

------- Comment #18 from dje at gcc dot gnu dot org  2008-05-02 17:36 -------
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?


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36090

[Bug target/36090] [4.3/4.4 Regression] ppc64 cacoshl miscompilation

[bonzini at gnu dot org]

------- Comment #19 from bonzini at gnu dot org  2008-05-02 21:08 -------
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.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36090

[Bug target/36090] [4.3/4.4 Regression] ppc64 cacoshl miscompilation

[dje at gcc dot gnu dot org]

------- Comment #20 from dje at gcc dot gnu dot org  2008-05-02 21:23 -------
How do we proceed?  Your initial patch is fine with me.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36090

[Bug target/36090] [4.3/4.4 Regression] ppc64 cacoshl miscompilation

[bonzini at gnu dot org]

------- Comment #21 from bonzini at gnu dot org  2008-05-03 07:13 -------
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


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36090

[Bug target/36090] [4.3/4.4 Regression] ppc64 cacoshl miscompilation

[dje at gcc dot gnu dot org]

------- 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).


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36090

[Bug target/36090] [4.3/4.4 Regression] ppc64 cacoshl miscompilation

[bonzini at gnu dot org]

------- Comment #23 from bonzini at gnu dot org  2008-05-03 13:25 -------
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


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36090

[Bug target/36090] [4.3/4.4 Regression] ppc64 cacoshl miscompilation

[dje at gcc dot gnu dot org]

------- 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.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36090

[Bug target/36090] [4.3/4.4 Regression] ppc64 cacoshl miscompilation

[bonzini at gnu dot org]

------- Comment #25 from bonzini at gnu dot org  2008-05-03 15:47 -------
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


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36090

[Bug target/36090] [4.3/4.4 Regression] ppc64 cacoshl miscompilation

[jakub at gcc dot gnu dot org]

------- Comment #26 from jakub at gcc dot gnu dot org  2008-05-04 19:47 -------
Created an attachment (id=15576)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=15576&action=view)
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*).


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36090

[Bug target/36090] [4.3/4.4 Regression] ppc64 cacoshl miscompilation

[dje at gcc dot gnu dot org]

------- Comment #27 from dje at gcc dot gnu dot org  2008-05-04 20:08 -------
I was planning to add asserts in print_operand_address ensuring that the
operand was the TOC SYMBOL_REF.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36090

[Bug target/36090] [4.3/4.4 Regression] ppc64 cacoshl miscompilation

[dje at gcc dot gnu dot org]

------- Comment #28 from dje at gcc dot gnu dot org  2008-05-08 16:36 -------
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


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36090

[Bug target/36090] [4.3/4.4 Regression] ppc64 cacoshl miscompilation

[dje at gcc dot gnu dot org]

------- Comment #29 from dje at gcc dot gnu dot org  2008-05-08 16:41 -------
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


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36090

[Bug target/36090] [4.3/4.4 Regression] ppc64 cacoshl miscompilation

[cnstar9988 at gmail dot com]

------- Comment #30 from cnstar9988 at gmail dot com  2008-05-09 05:36 -------
fixed?


-- 

cnstar9988 at gmail dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |cnstar9988 at gmail dot com


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36090

[Bug target/36090] [4.3/4.4 Regression] ppc64 cacoshl miscompilation

[jakub at gcc dot gnu dot org]

------- Comment #31 from jakub at gcc dot gnu dot org  2008-05-09 14:18 -------
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.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36090

[Bug target/36090] [4.3/4.4 Regression] ppc64 cacoshl miscompilation

[jakub at gcc dot gnu dot org]

------- Comment #32 from jakub at gcc dot gnu dot org  2008-05-09 14:32 -------
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));


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36090

[Bug target/36090] [4.3/4.4 Regression] ppc64 cacoshl miscompilation

[bonzini at gnu dot org]

------- Comment #33 from bonzini at gnu dot org  2008-05-09 15:51 -------
I now think that fixing it in the target is better.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36090

[Bug target/36090] [4.3/4.4 Regression] ppc64 cacoshl miscompilation

[dje at gcc dot gnu dot org]

------- Comment #34 from dje at gcc dot gnu dot org  2008-05-09 17:14 -------
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


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36090

[Bug target/36090] [4.3/4.4 Regression] ppc64 cacoshl miscompilation

[bonzini at gnu dot org]

------- Comment #35 from bonzini at gnu dot org  2008-05-12 15:50 -------
so you prefer to leave my fix (possibly amended in 4.4+)?


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36090

[Bug target/36090] [4.3/4.4 Regression] ppc64 cacoshl miscompilation

[dje at gcc dot gnu dot org]

------- Comment #36 from dje at gcc dot gnu dot org  2008-05-12 16:32 -------
Yes, I suggest keeping the simplify_plus_minus change with the CONST wrapper
removed.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36090

[Bug target/36090] [4.3/4.4 Regression] ppc64 cacoshl miscompilation

[bonzini at gnu dot org]

------- Comment #37 from bonzini at gnu dot org  2008-05-12 16:48 -------
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.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36090

[Bug target/36090] [4.3/4.4 Regression] ppc64 cacoshl miscompilation

[dje at gcc dot gnu dot org]

------- Comment #38 from dje at gcc dot gnu dot org  2008-05-12 16:51 -------
I did update the comment when I applied the patch.

I will work on this more when I return from travel next week.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36090

[Bug target/36090] [4.3/4.4 Regression] ppc64 cacoshl miscompilation

[bonzini at gnu dot org]

------- Comment #39 from bonzini at gnu dot org  2008-05-12 16:54 -------
Ah, only on 4.3 branch.  Ok, I'll go on from here.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36090

[Bug target/36090] [4.3/4.4 Regression] ppc64 cacoshl miscompilation

[rguenth at gcc dot gnu dot org]

------- Comment #40 from rguenth at gcc dot gnu dot org  2008-05-16 19:44 -------
What is the status of this bug on the 4.3 branch?


-- 

rguenth at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
      Known to fail|                            |4.3.0


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36090

[Bug target/36090] [4.3/4.4 Regression] ppc64 cacoshl miscompilation

[jakub at gcc dot gnu dot org]

------- Comment #41 from jakub at gcc dot gnu dot org  2008-05-18 18:31 -------
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.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36090

[Bug target/36090] [4.3/4.4 Regression] ppc64 cacoshl miscompilation

[jakub at gcc dot gnu dot org]

------- Comment #42 from jakub at gcc dot gnu dot org  2008-05-18 20:19 -------
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


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36090

[Bug target/36090] [4.3/4.4 Regression] ppc64 cacoshl miscompilation

[jakub at gcc dot gnu dot org]

------- Comment #43 from jakub at gcc dot gnu dot org  2008-05-18 20:20 -------
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


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36090

[Bug target/36090] [4.3/4.4 Regression] ppc64 cacoshl miscompilation

[jakub at gcc dot gnu dot org]

------- Comment #44 from jakub at gcc dot gnu dot org  2008-05-18 20:24 -------
The bug I've reported is fixed both on the trunk and on the branch, although
with different patches.


-- 

jakub at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|                            |FIXED


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36090