[Bug middle-end/26316] New: loop-invariant miscompiles openmp.c

[Bug middle-end/26316] New: loop-invariant miscompiles openmp.c

[jakub at gcc dot gnu dot org]

__attribute__ ((noinline))
void
bar (int i)
{
  asm volatile ("" : : "r" (i));
}

struct S
{
  int v;
  struct S *w;
};

__attribute__ ((noinline))
void
foo (struct S *s, int l)
{
  for (; s; s = s->w)
    {
      if (s->v && (l < 6 || l > 17))
        bar (1);
      switch (l)
        {
        case 6:
        case 7:
        case 8:
          bar (2);
          break;
        case 9:
        case 10:
          bar (3);
          break;
        case 13:
        case 14:
          bar (4);
          break;
        case 17:
          bar (5);
          break;
        default:
          break;
        }
    }
}

int
main (void)
{
  struct S s = { 0, 0 };
  foo (&s, 0);
  return 0;
}

is miscompiled on x86_64 at -O1 and above.  Before *r.loop2_invariant dump
(insn 30 29 31 6 o.i:22 (parallel [
            (set (reg:SI 61)
                (plus:SI (reg/v:SI 59 [ l ])
                    (const_int -6 [0xfffffffffffffffa])))
            (clobber (reg:CC 17 flags))
        ]) 208 {*addsi_1} (nil)
    (nil))

(insn 31 30 32 6 o.i:22 (set (reg:CC 17 flags)
        (compare:CC (reg:SI 61)
            (const_int 11 [0xb]))) 5 {*cmpsi_1_insn} (nil)
    (nil))

(jump_insn 32 31 85 6 o.i:22 (set (pc)
        (if_then_else (gtu (reg:CC 17 flags)
                (const_int 0 [0x0]))
            (label_ref 70)
            (pc))) 511 {*jcc_1} (nil)
    (expr_list:REG_BR_PROB (const_int 5000 [0x1388])
        (nil)))

(insn 33 85 35 7 o.i:22 (set (reg:DI 62)
        (zero_extend:DI (reg:SI 61))) 111 {zero_extendsidi2_rex64} (nil)
    (nil))

(insn 35 33 36 7 o.i:22 (set (reg:DI 64)
        (mem/u/c:DI (plus:DI (mult:DI (reg:DI 62)
                    (const_int 8 [0x8]))
                (label_ref:DI 38)) [0 S8 A8])) 81 {*movdi_1_rex64} (nil)
    (insn_list:REG_LABEL 38 (nil)))

(jump_insn 36 35 41 7 o.i:22 (parallel [
            (set (pc)
                (reg:DI 64))
            (use (label_ref 38))
        ]) 530 {*tablejump_1_rtx64} (nil)
    (nil))

so insn 35 isn't executed if (unsigned int) (l - 6) > 11.
But in *r.loop2_invariant dump this becomes:
(insn 19 13 33 3 o.i:20 (parallel [
            (set (reg:SI 65)
                (plus:SI (reg/v:SI 59 [ l ])
                    (const_int -6 [0xfffffffffffffffa])))
            (clobber (reg:CC 17 flags))
        ]) 208 {*addsi_1} (nil)
    (nil))

(insn 33 19 35 3 o.i:22 (set (reg:DI 66)
        (zero_extend:DI (reg:SI 65))) 111 {zero_extendsidi2_rex64} (nil)
    (nil))

(insn 35 33 92 3 o.i:22 (set (reg:DI 67)
        (mem/u/c:DI (plus:DI (mult:DI (reg:DI 66)
                    (const_int 8 [0x8]))
                (label_ref:DI 38)) [0 S8 A8])) 81 {*movdi_1_rex64} (nil)
    (insn_list:REG_LABEL 38 (nil)))


-- 
           Summary: loop-invariant miscompiles openmp.c
           Product: gcc
           Version: 4.2.0
            Status: UNCONFIRMED
          Severity: critical
          Priority: P3
         Component: middle-end
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: jakub at gcc dot gnu dot org
GCC target triplet: x86_64-linux


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

[Bug middle-end/26316] [4.2 Regression] loop-invariant miscompiles openmp.c

[pinskia at gcc dot gnu dot org]

-- 

pinskia at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |wrong-code
            Summary|loop-invariant miscompiles  |[4.2 Regression] loop-
                   |openmp.c                    |invariant miscompiles
                   |                            |openmp.c
   Target Milestone|---                         |4.2.0


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

[Bug middle-end/26316] [4.2 Regression] loop-invariant miscompiles openmp.c

[pinskia at gcc dot gnu dot org]

------- Comment #1 from pinskia at gcc dot gnu dot org  2006-02-16 12:50 -------
Confirmed but it might be that the memory load is not marked as being able to
trap so move invariants is moving it above the loop.


-- 

pinskia 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         |2006-02-16 12:50:22
               date|                            |


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

[Bug middle-end/26316] [4.2 Regression] loop-invariant miscompiles openmp.c

[jakub at gcc dot gnu dot org]

------- Comment #2 from jakub at gcc dot gnu dot org  2006-02-16 12:57 -------
It doesn't need to be marked as trapping.
If you have
  if (idx <= 20)
    val = p[idx];
you certainly can't change this into:
  tmp = p[idx];
  if (idx <= 20)
    val = tmp;
except if tmp = p[idx]; is speculative load that won't trap if the address
is invalid.


-- 


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

[Bug middle-end/26316] [4.2 Regression] loop-invariant miscompiles openmp.c

[rakdver at atrey dot karlin dot mff dot cuni dot cz]

------- Comment #3 from rakdver at atrey dot karlin dot mff dot cuni dot cz  2006-02-16 12:59 -------
Subject: Re:  [4.2 Regression] loop-invariant miscompiles openmp.c

> It doesn't need to be marked as trapping.
> If you have
>   if (idx <= 20)
>     val = p[idx];
> you certainly can't change this into:
>   tmp = p[idx];
>   if (idx <= 20)
>     val = tmp;
> except if tmp = p[idx]; is speculative load that won't trap if the address
> is invalid.

I certainly can change it this way if I know the memory is nontrapping.
In particular, if may_trap_p from some reason returns false for the
statement in question, that would be the cause of the problem.


-- 


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

[Bug middle-end/26316] [4.2 Regression] loop-invariant miscompiles openmp.c

[jakub at gcc dot gnu dot org]

------- Comment #4 from jakub at gcc dot gnu dot org  2006-02-16 13:11 -------
The MEM is marked as MEM_NOTRAP_P, by do_tablejump -> gen_const_mem.
Still, IMHO you can't do that, otherwise we couldn't mark any MEM
as MEM_NOTRAP_P unless it has constant operand.  If a particular array
will not trap, it doesn't mean you can add arbitrary offset to it and still
expect it will not trap.


-- 


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

[Bug middle-end/26316] [4.2 Regression] loop-invariant miscompiles openmp.c

[pinskia at gcc dot gnu dot org]

------- Comment #5 from pinskia at gcc dot gnu dot org  2006-02-16 13:35 -------
(In reply to comment #4)
> The MEM is marked as MEM_NOTRAP_P, by do_tablejump -> gen_const_mem.
> Still, IMHO you can't do that, otherwise we couldn't mark any MEM
> as MEM_NOTRAP_P unless it has constant operand.  If a particular array
> will not trap, it doesn't mean you can add arbitrary offset to it and still
> expect it will not trap.

Huh?
/* 1 if RTX is a mem that cannot trap.  */



@findex MEM_NOTRAP_P
@cindex @code{mem} and @samp{/c}
@cindex @code{call}, in @code{mem}
@item MEM_NOTRAP_P (@var{x})
In @code{mem}, nonzero for memory references that will not trap.
Stored in the @code{call} field and printed as @samp{/c}.

Since this reference can trap if it was pulled before the is, it should not be
marked as such.


-- 


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

[Bug middle-end/26316] [4.2 Regression] loop-invariant miscompiles openmp.c

[rakdver at gcc dot gnu dot org]

------- Comment #6 from rakdver at gcc dot gnu dot org  2006-02-16 13:38 -------
Sadly, Jakub is probably right -- since the very beginning, MEM_NOTRAP_P was
used to mean that the mem cannot trap in its current context.  This makes
MEM_NOTRAP_P completely useless as far as code motion is concerned (and I would
be very surprised if there were not some other uses of may_trap_p that are
wrong because of this).


-- 


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

[Bug middle-end/26316] [4.2 Regression] loop-invariant miscompiles openmp.c

[pinskia at gcc dot gnu dot org]

------- Comment #7 from pinskia at gcc dot gnu dot org  2006-02-16 13:42 -------
http://gcc.gnu.org/ml/gcc/2005-09/msg00021.html


There are most likely other talk here too.


-- 


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

[Bug middle-end/26316] [4.2 Regression] loop-invariant miscompiles openmp.c

[pinskia at gcc dot gnu dot org]

------- Comment #8 from pinskia at gcc dot gnu dot org  2006-02-16 13:47 -------
/* Generate a memory referring to non-trapping constant memory.  */
Which might mean that do_tablejump is using it wrong.


-- 


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

[Bug middle-end/26316] [4.2 Regression] loop-invariant miscompiles openmp.c

[rakdver at gcc dot gnu dot org]

------- Comment #9 from rakdver at gcc dot gnu dot org  2006-02-16 13:53 -------
Given that do_tablejump was the very first setter of MEM_NOTRAP_P, it is not
likely that it would be the wrong one (unless all other setters that came later
chose the other possible semantics).


-- 


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

[Bug middle-end/26316] [4.2 Regression] loop-invariant miscompiles openmp.c

[pinskia at gcc dot gnu dot org]

------- Comment #10 from pinskia at gcc dot gnu dot org  2006-02-16 13:54 -------
The NOTRAP was added when PR 8866 was fixed.


-- 

pinskia at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  BugsThisDependsOn|                            |8866
           Keywords|monitored                   |


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

[Bug middle-end/26316] [4.2 Regression] loop-invariant miscompiles openmp.c

[pinskia at gcc dot gnu dot org]

------- Comment #11 from pinskia at gcc dot gnu dot org  2006-02-16 13:57 -------
Thread for the PR 8866 fix:
http://gcc.gnu.org/ml/gcc-patches/2003-04/msg01438.html

Though I am wondering why do people think it could not trap because it can if
it was moved.  Seems like NoTrap is useless in general code motion.


-- 


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

[Bug middle-end/26316] [4.2 Regression] loop-invariant miscompiles openmp.c

[jakub at gcc dot gnu dot org]

------- Comment #12 from jakub at gcc dot gnu dot org  2006-02-16 21:31 -------
Subject: Bug 26316

Author: jakub
Date: Thu Feb 16 21:31:20 2006
New Revision: 111152

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=111152
Log:
        * openmp.c (resolve_omp_clauses): Add a dummy case label to workaround
        PR middle-end/26316.

Modified:
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/openmp.c


-- 


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

[Bug middle-end/26316] [4.2 Regression] loop-invariant miscompiles openmp.c

[rakdver at gcc dot gnu dot org]

-- 

rakdver at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|unassigned at gcc dot gnu   |rakdver at gcc dot gnu dot
                   |dot org                     |org
             Status|NEW                         |ASSIGNED
   Last reconfirmed|2006-02-16 12:50:22         |2006-02-17 09:51:59
               date|                            |


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

[Bug middle-end/26316] [4.2 Regression] loop-invariant miscompiles openmp.c

[rakdver at gcc dot gnu dot org]

------- Comment #13 from rakdver at gcc dot gnu dot org  2006-02-17 18:38 -------
Patch:

http://gcc.gnu.org/ml/gcc-patches/2006-02/msg01461.html


-- 

rakdver at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                URL|                            |http://gcc.gnu.org/ml/gcc-
                   |                            |patches/2006-
                   |                            |02/msg01461.html
           Keywords|                            |patch


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

[Bug middle-end/26316] [4.2 Regression] loop-invariant miscompiles openmp.c

[rakdver at gcc dot gnu dot org]

------- Comment #14 from rakdver at gcc dot gnu dot org  2006-02-23 21:03 -------
Subject: Bug 26316

Author: rakdver
Date: Thu Feb 23 21:03:05 2006
New Revision: 111397

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=111397
Log:
        PR rtl-optimization/26316
        * rtlanal.c (enum may_trap_p_flags): New.
        (may_trap_p_1): Take flags instead of unaligned_mems argument.  Ignore
        MEM_NOTRAP_P if flags & MTP_AFTER_MOVE.
        (may_trap_p, may_trap_or_fault_p): Pass flags to may_trap_p_1.
        (may_trap_after_code_motion_p): New function.
        * loop-invariant.c (find_identical_invariants): Fix dump formating.
        (find_invariant_insn): Use may_trap_after_code_motion_p.
        * rtl.h (may_trap_after_code_motion_p): Declare.


Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/loop-invariant.c
    trunk/gcc/rtl.h
    trunk/gcc/rtlanal.c


-- 


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

[Bug middle-end/26316] [4.2 Regression] loop-invariant miscompiles openmp.c

[pinskia at gcc dot gnu dot org]

------- Comment #15 from pinskia at gcc dot gnu dot org  2006-02-24 15:20 -------
Fixed.


-- 

pinskia at gcc dot gnu dot org changed:

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


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

[Bug middle-end/26316] [4.2 Regression] loop-invariant miscompiles openmp.c

[reichelt at gcc dot gnu dot org]

------- Comment #16 from reichelt at gcc dot gnu dot org  2006-02-28 14:40 -------
Jakub,

are you going to revert your patch

        * openmp.c (resolve_omp_clauses): Add a dummy case label to workaround
        PR middle-end/26316.

now that the PR got fixed properly?


-- 


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