Re: loop optimizer naivety

[law at redhat dot com]

In message <200309230836.57001.ebotcazou@libertysurf.fr>, Eric Botcazou writes:
 >[Thanks for this answer]
 >
 >> Is there any path through the loop where insns 810 and 812 could both
 >> execute?  ie, can you get from insn 810 to 812 or vice versa without
 >> going back to the top of the loop?
 >
 >Yes, they belong to the same basic block (otherwise the loop optimizer 
 >wouldn't have made its mistake).
 >
 >The complete sequence is:
 >
 >(note 604 568 609 4 NOTE_INSN_LOOP_CONT)
 >
 >(insn 609 604 810 4 0x40a5565c (parallel [
 >            (set (reg:SI 173)
 >                (plus:SI (reg:SI 185)
 >                    (const_int 1 [0x1])))
 >            (clobber (reg:CC 17 flags))
 >        ]) 146 {*addsi_1} (nil)
 >    (nil))
 >
 >(insn 810 609 610 4 (nil) (set (reg:SI 185)
 >        (reg:SI 173)) 38 {*movsi_1} (nil)
 >    (nil))
 >
 >(insn 610 810 765 4 0x40a5565c (set (mem/f:SI (plus:SI (reg/f:SI 20 frame)
 >                (const_int -100 [0xffffff9c])) [5 S4 A32])
 >        (reg:SI 173)) 38 {*movsi_1} (nil)
 >    (expr_list:REG_EQUAL (plus:SI (reg:SI 161)
 >            (const_int 1 [0x1]))
 >        (nil)))
 >
 >(note 765 610 812 4 NOTE_INSN_LOOP_VTOP)
 >
 >(insn 812 765 103 4 (nil) (set (reg:SI 185)
 >        (reg:SI 173)) 38 {*movsi_1} (nil)
 >    (nil))
 >
 >
 >insn 812 is dead (reg 185 is not consumed) but delete_trivially_dead_insns() 
 >deletes only _really_ trivially dead insns :-)
 >
 >
 >Before the GCSE pass, the sequence is:
 >
 >(insn 609 604 610 3 0x40a5565c (parallel [
 >            (set (reg:SI 173)
 >                (plus:SI (reg:SI 161)
 >                    (const_int 1 [0x1])))
 >            (clobber (reg:CC 17 flags))
 >        ]) 146 {*addsi_1} (nil)
 >    (nil))
 >
 >(insn 610 609 765 3 0x40a5565c (set (mem/f:SI (plus:SI (reg/f:SI 20 frame)
 >                (const_int -100 [0xffffff9c])) [5 S4 A32])
 >        (reg:SI 173)) 38 {*movsi_1} (nil)
 >    (expr_list:REG_EQUAL (plus:SI (reg:SI 161)
 >            (const_int 1 [0x1]))
 >        (nil)))
 >
 >(note 765 610 102 3 NOTE_INSN_LOOP_VTOP)
 >
 >(insn 102 765 103 3 0x40a5565c (set (reg/s:SI 89)
 >        (mem/f:SI (plus:SI (reg/f:SI 20 frame)
 >                (const_int -100 [0xffffff9c])) [5 S4 A32])) 38 {*movsi_1} 
 >(nil)
 >    (nil))
 >
 >
 >and reg 89 is consumed.
 >
 >
 >> If so, then PRE/LCM has mucked things up.
 >
 >Then I should fix GCSE and not the loop optimizer?
Well, PRE/LCM is doing something goofy, though thinking about it more closely,
I can speculate what might be happening and it would be within the realm of
correctness (though not necessarily optimal).

loop is definitely broken since it's making some insanely lame assumptions
about the structure of the code.

I would speculate that after PRE/LCM was completed that insn 610 referenced
reg 185 rather than 173.  ie, the code looked something like this:


 (note 604 568 609 4 NOTE_INSN_LOOP_CONT)

(insn 609 604 810 4 0x40a5565c (parallel [
            (set (reg:SI 173)
                (plus:SI (reg:SI 185)
                    (const_int 1 [0x1])))
            (clobber (reg:CC 17 flags))
        ]) 146 {*addsi_1} (nil)
    (nil))

(insn 810 609 610 4 (nil) (set (reg:SI 185)
        (reg:SI 173)) 38 {*movsi_1} (nil)
    (nil))

(insn 610 810 765 4 0x40a5565c (set (mem/f:SI (plus:SI (reg/f:SI 20 frame)
                (const_int -100 [0xffffff9c])) [5 S4 A32])
        (reg:SI 185)) 38 {*movsi_1} (nil)
                ^^^
    (expr_list:REG_EQUAL (plus:SI (reg:SI 161)
            (const_int 1 [0x1]))
        (nil)))

(note 765 610 812 4 NOTE_INSN_LOOP_VTOP)

(insn 812 765 103 4 (nil) (set (reg:SI 185)
        (reg:SI 173)) 38 {*movsi_1} (nil)
    (nil))


Then copy-prop replaced reg 185 with reg 173 in insn 610, which
leads to the somewhat odd looking code in your message.


If you could verify that sequence of events it would be helpful in at
least determining that LCM is operating properly.  To do this you'd put
a breakpoint just before this hunk of code in gcse.c and dump the RTL
to a file:

      if (max_pass_bytes < bytes_used)
        max_pass_bytes = bytes_used;

      /* Free up memory, then reallocate for code hoisting.  We can
         not re-use the existing allocated memory because the tables
         will not have info for the insns or registers created by
         partial redundancy elimination.  */
      free_gcse_mem ();





Note this code is contained inside a loop.  So if you don't see the code
above, see if the we loop and run PRE again, then see if you get
the expected code the second time around.




Jeff