[Bug rtl-optimization/57159] Latent bug in RTL GCSE/PRE

[Bug rtl-optimization/57159] Latent bug in RTL GCSE/PRE

[jules at gcc dot gnu.org]

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

--- Comment #1 from jules at gcc dot gnu.org 2013-05-03 11:56:53 UTC ---
Created attachment 30019
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=30019
patch

[Bug rtl-optimization/57159] New: Latent bug in RTL GCSE/PRE

[jules at gcc dot gnu.org]

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

             Bug #: 57159
           Summary: Latent bug in RTL GCSE/PRE
    Classification: Unclassified
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Severity: minor
          Priority: P3
         Component: rtl-optimization
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: jules@gcc.gnu.org


Created attachment 30018
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=30018
testcase

We encountered a wrong-code bug in an out-of-tree port (4.6-based, though the
affected code doesn't appear to have changed that much) in target-independent
code, though I have not been able to reproduce the problem on a
currently-supported target. The port in question is unusual in that it allows
read-modify-write arithmetic directly on memory locations (for some operations,
at least).

That means that during RTL expansion, the compiler may attach REG_EQUAL notes
to insns that contain memory operands:

(insn 16 15 17 (set (reg:SI 70)
        (mem/s:SI (reg/v/f:SI 68 [ iter ]) [3 iter_4(D)->xhv_riter+0 S4 A32]))
hvmin.c:32 -1
     (nil))

(insn 17 16 0 (set (reg:SI 56 [ D.4066 ])
        (plus:SI (reg:SI 70)
            (const_int 1 [0x1]))) hvmin.c:32 -1
     (expr_list:REG_EQUAL (plus:SI (mem/s:SI (reg/v/f:SI 68 [ iter ]) [3
iter_4(D)->xhv_riter+0 S4 A32])
            (const_int 1 [0x1]))
        (nil)))

That in itself is not a problem, but certain parts of gcse.c's PRE code examine
only the instruction itself (e.g. when building tables of "interesting" memory
locations -- compute_ld_motion_mems), whereas other parts pay attention to the
REG_EQUAL note (hash_scan_set). That means that memory references which are
"too complicated" for PRE to handle properly can leak through to later parts of
the algorithm and cause misoptimisations -- in the case of the attached file,
the increment in the loop is optimised away, and the loop spins forever.

The fix then is to prevent "complicated" memory references which may be present
in REG_EQUAL notes from being considered for load motion, by stopping them from
being added to the relevant tables to start with. That's what the attached
patch does, and it works for us -- though without a way of reproducing the bug
on mainline, it's not quite obvious that it should be applied there.

Interestingly, on x86 (which also allows read-modify-write operations), it
looks like the bug is latent because a normal addition clobbers the flags
register:

(insn 28 91 94 4 (parallel [
            (set (reg:SI 60 [ D.2122 ])
                (plus:SI (reg:SI 74 [ iter_4(D)->xhv_riter ])
                    (const_int 1 [0x1])))
            (clobber (reg:CC 17 flags))
        ]) hvmin.c:32 252 {*addsi_1}
     (expr_list:REG_DEAD (reg:SI 74 [ iter_4(D)->xhv_riter ])
        (expr_list:REG_UNUSED (reg:CC 17 flags)
            (expr_list:REG_EQUAL (plus:SI (mem/s:SI (reg/v/f:DI 72 [ iter ]) [3
iter_4(D)->xhv_riter+0 S4 A32])
                    (const_int 1 [0x1]))
                (nil)))))

This is apparently sufficient to prevent the misoptimisation (probably because
of want_to_gcse_p's call to can_assign_to_reg_without_clobbers_p). I tried to
reproduce on m68k also, but addsi patterns are expanded differently there so
the failing condition doesn't trigger.

[Bug rtl-optimization/57159] Latent bug in RTL GCSE/PRE

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |stevenb.gcc at gmail dot
                   |                            |com

--- Comment #2 from Richard Biener <rguenth at gcc dot gnu.org> 2013-05-03 12:31:27 UTC ---
Patches should go to gcc-patches.  Eventually Steven knows this code best.

[Bug rtl-optimization/57159] Latent bug in RTL GCSE/PRE

[law at redhat dot com]

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

Jeffrey A. Law <law at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |law at redhat dot com

--- Comment #3 from Jeffrey A. Law <law at redhat dot com> 2013-05-03 18:29:13 UTC ---
It'd really help if you could add the .cprop1 & .pre dumps before/after your
change.

[Bug rtl-optimization/57159] Latent bug in RTL GCSE/PRE

[jules at gcc dot gnu.org]

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

--- Comment #4 from jules at gcc dot gnu.org 2013-05-03 19:56:33 UTC ---
Created attachment 30029
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=30029
Before/after dumps

Here are some before/after dumps taken from the out-of-tree port. Notice how in
the "before" pre dump, the results of insns 27 and 28 are considered to be
equivalent to the results of insns 16 and 17, despite the store (insn 18)
making that equivalence invalid -- this is the misoptimisation alluded to
earlier.

This happens because oprs_available_p mistakenly thinks that insn 16/17's
results are available at the end of their BB, because oprs_unchanged_p ->
load_killed_in_block_p -> mems_conflict_for_gcse_p -> find_rtx_in_ldst finds a
load/store that it thinks it can deal with ("(mem/s:SI (reg/v/f:SI 68 [ iter
]))"), but actually that mem is used (in the REG_EQUAL note, as extracted by
hash_scan_set) in a way that it cannot handle.

[Bug rtl-optimization/57159] Latent bug in RTL GCSE/PRE

[jules at gcc dot gnu.org]

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

--- Comment #5 from jules at gcc dot gnu.org 2013-05-03 20:05:19 UTC ---
Actually the last paragraph might not make sense -- insn 16/17's *operands* are
not available at the end of the BB, but only if the REG_EQUAL note contents are
substituted for the insn pattern. But that's kind of what happens in
hash_scan_set, so I think the overall idea's right.

[Bug rtl-optimization/57159] Latent bug in RTL GCSE/PRE

[steven at gcc dot gnu.org]

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

Steven Bosscher <steven at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #30019|0                           |1
        is obsolete|                            |

--- Comment #6 from Steven Bosscher <steven at gcc dot gnu.org> ---
Comment on attachment 30019
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=30019
patch

Committed in r199049

[Bug rtl-optimization/57159] Latent bug in RTL GCSE/PRE

[steven at gcc dot gnu.org]

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

Steven Bosscher <steven at gcc dot gnu.org> changed:

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

--- Comment #7 from Steven Bosscher <steven at gcc dot gnu.org> ---
Jules, please kindly close your fixed bugs after yourself ;-)