[Bug target/42431] New: wrong code for 200.sixtrack with vectorization and -fdata-sections

[Bug target/42431] New: wrong code for 200.sixtrack with vectorization and -fdata-sections

[janis at gcc dot gnu dot org]

When compiled with current trunk and options "-m64 -O2 -ftree-vectorize
-maltivec -fdata-sections", CPU2000 test 200.sixtrack gets wrong results and
segfaults before finishing.  I've reproduced the failure on POWER6 and POWER7
hardware.  The source file that is miscompiled is midbloc6.f.  If I add this
line:

  if (icount1.eq.187) write (*,*) ' howdy'

to any two of the DO loops associated with labels 20, 40, 50, 110, 200, 210,
220, 260, and 280 then the test runs to completion and gets the expected
output.

The test passes with these options with GCC 4.4.0.  I don't currently have a
reg
hunt setup on a machine with AltiVec but can do that it if would be useful.


-- 
           Summary: wrong code for 200.sixtrack with vectorization and -
                    fdata-sections
           Product: gcc
           Version: 4.5.0
            Status: UNCONFIRMED
          Keywords: wrong-code
          Severity: normal
          Priority: P3
         Component: target
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: janis at gcc dot gnu dot org
GCC target triplet: powerpc64-linux


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

[Bug target/42431] wrong code for 200.sixtrack with vectorization and -fdata-sections

[janis at gcc dot gnu dot org]

------- Comment #1 from janis at gcc dot gnu dot org  2009-12-18 23:03 -------
Oh yeah, 176.gcc fails with the same options.  With test input it runs for a
few minutes instead of a couple of seconds, and the results are wrong.


-- 


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

[Bug target/42431] [4.5 Regression] wrong code for 200.sixtrack with vectorization and -fdata-sections

[rguenth at gcc dot gnu dot org]

-- 

rguenth at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|wrong code for 200.sixtrack |[4.5 Regression] wrong code
                   |with vectorization and -    |for 200.sixtrack with
                   |fdata-sections              |vectorization and -fdata-
                   |                            |sections
   Target Milestone|---                         |4.5.0


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

[Bug target/42431] [4.5 Regression] wrong code for 200.sixtrack with vectorization and -fdata-sections

[rguenth at gcc dot gnu dot org]

------- Comment #2 from rguenth at gcc dot gnu dot org  2010-01-02 16:06 -------
Testcaese?


-- 

rguenth at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |WAITING


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

[Bug target/42431] [4.5 Regression] wrong code for 200.sixtrack with vectorization and -fdata-sections

[janis at gcc dot gnu dot org]

------- Comment #3 from janis at gcc dot gnu dot org  2010-01-04 18:06 -------
No testcase yet, I was hoping to get bergner or meissner to look at it since
they have access to the benchmark.


-- 


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

[Bug target/42431] [4.5 Regression] wrong code for 200.sixtrack with vectorization and -fdata-sections

[janis at gcc dot gnu dot org]

------- Comment #4 from janis at gcc dot gnu dot org  2010-02-09 22:38 -------
Peter and Mike, can one of you please look at this bug?  It looks like
something that you could figure out without a minimized testcase, but if it
helps I'll come up with one.  I can also do a regression hunt if that would
help, but I'd have to set things up on a new machine.


-- 


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

[Bug target/42431] [4.5 Regression] wrong code for 200.sixtrack with vectorization and -fdata-sections

[janis at gcc dot gnu dot org]

------- Comment #6 from janis at gcc dot gnu dot org  2010-02-12 20:36 -------
In the attached testcase wrong code is generated for the loop in subroutine
sub3 that sets 12 of the elements of array k to zero.  While minimizing the
testcase I saw different kinds of bad code for that array, but with this
version it's:

        vxor 31,31,31
        ld 9,.LC2@toc(2)
        ld 29,.LC2@toc(2)
        li 0,16 
        stvx 31,0,29
        add 9,9,9
        stvx 31,0,9
        stvx 31,9.0

Needless to say, doubling the array address has bad consequences.  It would
make
 sense if the instruction were instead "add 9,29,0"

I'm planning to stare at dump files for awhile but won't complain if someone
else figures it out first.


-- 


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

[Bug target/42431] [4.5 Regression] wrong code for 200.sixtrack with vectorization and -fdata-sections

[janis at gcc dot gnu dot org]

------- Comment #5 from janis at gcc dot gnu dot org  2010-02-12 20:35 -------
Created an attachment (id=19853)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=19853&action=view)
minimized executable testcase

Compile with "-m64 -O2 -maltivec -ftree-vectorize -fdata-sections".


-- 


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

[Bug target/42431] [4.5 Regression] wrong code for 200.sixtrack with vectorization and -fdata-sections

[pthaugen at gcc dot gnu dot org]

------- Comment #7 from pthaugen at gcc dot gnu dot org  2010-02-12 23:16 -------
This looks like an ira/reload bug.  The ira dump is where the offending insns
first show up.

(insn 17 26 244 3 pr42431.f:27 (set (mem:V4SI (reg/f:DI 29 29 [255]) [4 S16
A128])
        (reg:V4SI 108 31 [269])) 942 {*altivec_movv4si} (expr_list:REG_EQUAL
(const_vector:V4SI [
                (const_int 0 [0x0])
                (const_int 0 [0x0])
                (const_int 0 [0x0])
                (const_int 0 [0x0])
            ])
        (nil)))

(insn 244 17 245 3 pr42431.f:27 (set (reg:DI 9 9)
        (mem/u/c:DI (plus:DI (reg:DI 2 2)
                (const:DI (unspec:DI [
                            (symbol_ref/u:DI ("*.LC2") [flags 0x2])
                        ] 49))) [7 S8 A8])) 373 {*movdi_internal64} (nil))

(insn 245 244 20 3 pr42431.f:27 (set (reg:DI 9 9)
        (plus:DI (reg:DI 9 9)
            (reg:DI 9 9))) 83 {*adddi3_internal1} (nil))

(insn 20 245 246 3 pr42431.f:27 (set (mem:V4SI (reg:DI 9 9) [4 S16 A128])
        (reg:V4SI 108 31 [269])) 942 {*altivec_movv4si} (expr_list:REG_EQUAL
(const_vector:V4SI [
                (const_int 0 [0x0])
                (const_int 0 [0x0])
                (const_int 0 [0x0])
                (const_int 0 [0x0])
            ])
        (nil)))


insns 244/245 are not present in the prior (sched1) dump. insn 245 should be
adding the constant 16 to gpr9 instead of adding r9.


-- 

pthaugen at gcc dot gnu dot org changed:

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


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

[Bug target/42431] [4.5 Regression] wrong code for 200.sixtrack with vectorization and -fdata-sections

[bergner at gcc dot gnu dot org]

------- Comment #8 from bergner at gcc dot gnu dot org  2010-02-16 20:02 -------
Thanks for the reduced testcase.  I'll have a look at what's wrong.


-- 


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

[Bug target/42431] [4.5 Regression] wrong code for 200.sixtrack with vectorization and -fdata-sections

[mmitchel at gcc dot gnu dot org]

-- 

mmitchel at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P1


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

[Bug target/42431] [4.5 Regression] wrong code for 200.sixtrack with vectorization and -fdata-sections

[rguenth at gcc dot gnu dot org]

-- 

rguenth at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|WAITING                     |NEW
     Ever Confirmed|0                           |1
   Last reconfirmed|0000-00-00 00:00:00         |2010-02-22 12:40:53
               date|                            |


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

[Bug middle-end/42431] [4.5 Regression] wrong code for 200.sixtrack with vectorization and -fdata-sections

[bergner at gcc dot gnu dot org]

------- Comment #9 from bergner at gcc dot gnu dot org  2010-02-23 21:57 -------
This is a generic reload bug.  With this test case and options, we have the
following instructions just before IRA:

(insn 203 2 213 2 (set (reg/f:DI 256 [ vect_pk.44 ])
        (plus:DI (reg/f:DI 255)
            (const_int 16 [0x10]))) 83 {*adddi3_internal1} (expr_list:REG_EQUAL
(const:DI (plus:DI (symbol_ref:DI ("*.LANCHOR2") [flags 0x182])
                (const_int 16 [0x10])))
        (nil)))
 ...
(insn 21 214 25 2 pr42431.f:27 (set (reg:DI 270)
        (const_int 16 [0x10])) 371 {*movdi_internal64} (nil))
...
(insn 20 17 23 3 pr42431.f:27 (set (mem:V4SI (reg/f:DI 256 [ vect_pk.44 ]) [4
S16 A128])
        (reg:V4SI 269)) 926 {*altivec_movv4si} (expr_list:REG_EQUAL
(const_vector:V4SI [
                (const_int 0 [0x0])
                (const_int 0 [0x0])
                (const_int 0 [0x0])
                (const_int 0 [0x0])
            ])
        (nil)))

(insn 23 20 232 3 pr42431.f:27 (set (mem:V4SI (plus:DI (reg/f:DI 256 [
vect_pk.44 ])
                (reg:DI 270)) [4 S16 A128])
        (reg:V4SI 269)) 926 {*altivec_movv4si} (expr_list:REG_EQUAL
(const_vector:V4SI [
                (const_int 0 [0x0])
                (const_int 0 [0x0])
                (const_int 0 [0x0])
                (const_int 0 [0x0])
            ])
        (nil)))

Both r256 and r270 are marked for spilling.  This leads to two chained reloads:

  (const:DI (plus:DI (symbol_ref:DI ("*.LANCHOR2") [flags 0x182]) (const_int 16
[0x10])))
and
  (const_int 16 [0x10])

With these chained reloads, we eventually drop into
gen_reload_chain_without_interm_reg_p().  This function does a copy_rtx() to
create a scratch rtx it can modify to run some tests on and then throw away. 
The bug is that copy_rtx() simply returns the passed in rtx pointer for CONST
rtxs, so we end up modifying the program's real rtl rather than some scratch
rtl.  Since we cannot execute the copy_rtx/substitue statements without causing
problems, I'm testing the patch below to see if it fixes the
problem...although, if there are CONSTs buried deep within the reload rtx, we
probably could still end up with a similar problem on a different test case.
Comments anyone?

Index: reload1.c
===================================================================
--- reload1.c   (revision 156816)
+++ reload1.c   (working copy)
@@ -5221,6 +5221,10 @@
       r1 = r2;
       r2 = n;
     }
+
+  if (GET_CODE (rld[r1].in) == CONST)
+    return true;
+
   gcc_assert (reg_mentioned_p (rld[r2].in, rld[r1].in));
   regno = rld[r1].regno >= 0 ? rld[r1].regno : rld[r2].regno;
   gcc_assert (regno >= 0);


-- 

bergner at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |vmakarov at gcc dot gnu dot
                   |                            |org
          Component|target                      |middle-end


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

[Bug middle-end/42431] [4.5 Regression] wrong code for 200.sixtrack with vectorization and -fdata-sections

[meissner at linux dot vnet dot ibm dot com]

------- Comment #10 from meissner at linux dot vnet dot ibm dot com  2010-02-24 21:01 -------
Subject: Re:  [4.5 Regression] wrong code for
 200.sixtrack with vectorization and -fdata-sections

On Tue, Feb 23, 2010 at 09:57:17PM -0000, bergner at gcc dot gnu dot org wrote:
> 
> 
> ------- Comment #9 from bergner at gcc dot gnu dot org  2010-02-23 21:57 -------
> This is a generic reload bug.  With this test case and options, we have the
> following instructions just before IRA:
> 
> (insn 203 2 213 2 (set (reg/f:DI 256 [ vect_pk.44 ])
>         (plus:DI (reg/f:DI 255)
>             (const_int 16 [0x10]))) 83 {*adddi3_internal1} (expr_list:REG_EQUAL
> (const:DI (plus:DI (symbol_ref:DI ("*.LANCHOR2") [flags 0x182])
>                 (const_int 16 [0x10])))
>         (nil)))
>  ...
> (insn 21 214 25 2 pr42431.f:27 (set (reg:DI 270)
>         (const_int 16 [0x10])) 371 {*movdi_internal64} (nil))
> ...
> (insn 20 17 23 3 pr42431.f:27 (set (mem:V4SI (reg/f:DI 256 [ vect_pk.44 ]) [4
> S16 A128])
>         (reg:V4SI 269)) 926 {*altivec_movv4si} (expr_list:REG_EQUAL
> (const_vector:V4SI [
>                 (const_int 0 [0x0])
>                 (const_int 0 [0x0])
>                 (const_int 0 [0x0])
>                 (const_int 0 [0x0])
>             ])
>         (nil)))
> 
> (insn 23 20 232 3 pr42431.f:27 (set (mem:V4SI (plus:DI (reg/f:DI 256 [
> vect_pk.44 ])
>                 (reg:DI 270)) [4 S16 A128])
>         (reg:V4SI 269)) 926 {*altivec_movv4si} (expr_list:REG_EQUAL
> (const_vector:V4SI [
>                 (const_int 0 [0x0])
>                 (const_int 0 [0x0])
>                 (const_int 0 [0x0])
>                 (const_int 0 [0x0])
>             ])
>         (nil)))
> 
> Both r256 and r270 are marked for spilling.  This leads to two chained reloads:
> 
>   (const:DI (plus:DI (symbol_ref:DI ("*.LANCHOR2") [flags 0x182]) (const_int 16
> [0x10])))
> and
>   (const_int 16 [0x10])
> 
> With these chained reloads, we eventually drop into
> gen_reload_chain_without_interm_reg_p().  This function does a copy_rtx() to
> create a scratch rtx it can modify to run some tests on and then throw away. 
> The bug is that copy_rtx() simply returns the passed in rtx pointer for CONST
> rtxs, so we end up modifying the program's real rtl rather than some scratch
> rtl.  Since we cannot execute the copy_rtx/substitue statements without causing
> problems, I'm testing the patch below to see if it fixes the
> problem...although, if there are CONSTs buried deep within the reload rtx, we
> probably could still end up with a similar problem on a different test case.
> Comments anyone?

I suspect this may have been the culprit for the bandaid that I put in
rs6000_emit_move (around line 6339 of rs6000.c) to handle const addresses that
were overwritten with a register.  Here is the comment that starts the block:

  /* Fix up invalid (const (plus (symbol_ref) (reg))) that seems to be created
     in the secondary_reload phase, which evidently overwrites the CONST_INT
     with a register.  */


> Index: reload1.c
> ===================================================================
> --- reload1.c   (revision 156816)
> +++ reload1.c   (working copy)
> @@ -5221,6 +5221,10 @@
>        r1 = r2;
>        r2 = n;
>      }
> +
> +  if (GET_CODE (rld[r1].in) == CONST)
> +    return true;
> +
>    gcc_assert (reg_mentioned_p (rld[r2].in, rld[r1].in));
>    regno = rld[r1].regno >= 0 ? rld[r1].regno : rld[r2].regno;
>    gcc_assert (regno >= 0);
> 
> 

I dunno, would it be better if we cloned copy_rtx to have a varient that
doesn't call shared_const_p in rtl.c and made a full copy?  I would think this
would be better than returning true if it is a constant.

If we decide to keep the current code, please put a comment in re-iterating the
reasons mentioned above (and maybe a call to shared_const_p to see if copy_rtx
would copy it).


-- 


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

[Bug middle-end/42431] [4.5 Regression] wrong code for 200.sixtrack with vectorization and -fdata-sections

[bergner at vnet dot ibm dot com]

------- Comment #11 from bergner at vnet dot ibm dot com  2010-02-24 22:08 -------
Subject: Re:  [4.5 Regression] wrong code for
 200.sixtrack with vectorization and -fdata-sections

On Wed, 2010-02-24 at 21:01 +0000, meissner at linux dot vnet dot ibm dot com
wrote:
> 
> ------- Comment #10 from meissner at linux dot vnet dot ibm dot com  2010-02-24 21:01 -------
[snip]
> I suspect this may have been the culprit for the bandaid that I put in
> rs6000_emit_move (around line 6339 of rs6000.c) to handle const addresses that
> were overwritten with a register.  Here is the comment that starts the block:
> 
>   /* Fix up invalid (const (plus (symbol_ref) (reg))) that seems to be created
>      in the secondary_reload phase, which evidently overwrites the CONST_INT
>      with a register.  */

It sounds like the same problem.   Once this is fixed, we probably can back
that change out after verifying we don't need it anymore.



> > Index: reload1.c
> > ===================================================================
> > --- reload1.c   (revision 156816)
> > +++ reload1.c   (working copy)
> > @@ -5221,6 +5221,10 @@
> >        r1 = r2;
> >        r2 = n;
> >      }
> > +
> > +  if (GET_CODE (rld[r1].in) == CONST)
> > +    return true;
> > +
> >    gcc_assert (reg_mentioned_p (rld[r2].in, rld[r1].in));
> >    regno = rld[r1].regno >= 0 ? rld[r1].regno : rld[r2].regno;
> >    gcc_assert (regno >= 0);
> > 
> > 
> 
> I dunno, would it be better if we cloned copy_rtx to have a varient that
> doesn't call shared_const_p in rtl.c and made a full copy?  I would think this
> would be better than returning true if it is a constant.

I asked that same question in my post to gcc-patches, except I _think_ we
need to copy the other sharable rtx objects too, not just const's.

Peter


-- 


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

[Bug middle-end/42431] [4.5 Regression] wrong code for 200.sixtrack with vectorization and -fdata-sections

[bergner at gcc dot gnu dot org]

------- Comment #12 from bergner at gcc dot gnu dot org  2010-03-04 19:08 -------
This bug was fixed with Jeff Law's commit (revision 157168) of the patch
attached to the Patch URL listed above.


-- 

bergner at gcc dot gnu dot org changed:

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


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

[Bug middle-end/42431] [4.5 Regression] wrong code for 200.sixtrack with vectorization and -fdata-sections

[bergner at gcc dot gnu dot org]

------- Comment #13 from bergner at gcc dot gnu dot org  2010-03-04 19:59 -------
I verified this is fixed with a fresh checkout of mainline.


-- 

bergner at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |VERIFIED


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

[Bug middle-end/42431] [4.5 Regression] wrong code for 200.sixtrack with vectorization and -fdata-sections

[meissner at gcc dot gnu dot org]

------- Comment #14 from meissner at gcc dot gnu dot org  2010-03-11 20:11 -------
Created an attachment (id=20090)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=20090&action=view)
Patch to remove band-aid now that the underlying problem was fixed

In doing the initial VSX development, I ran into a situation in building spec
2006, that ultimately was caused by the bug in 42431.  I added some band-aid
code to make sure the problem was not seen.  After Jeff's patches were checked
in, I deleted the code, and built spec 2006 with several different options, and
the code built/ran correctly.  So this is no longer needed.


-- 


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