Reload bug causing x86 problems

[Jeffrey A Law <law at upchuck dot cygnus dot com>]

This change:

Fri Oct 16 20:40:50 1998  J"orn Rennecke  <amylaar@cygnus.co.uk>

        Fix consistency problems with reg_equiv_{mem,address};
        Improve reload inheritance;

        * reload.c (reload_out_reg): New variable.
        (loc_mentioned_in_p, remove_address_replacements): New functions.
        (remove_replacements): Deleted.
        (push_reload): Set reload_out_reg[i].
        When merging, also set reload_{in,out}_reg[i], and remove
        duplicate address reloads.


Is causing some serious problems on the x86.

Given the following insn::

(insn 98 102 26 (set (reg/v:SI 1 %edx)
        (if_then_else:SI (ge:SI (reg/v:SI 0 %eax)
                (mem/s:SI (plus:SI (reg/v:SI 22)
                        (const_int 4 [0x4])) 2))
            (reg/v:SI 1 %edx)
            (mem/s:SI (plus:SI (reg/v:SI 22)
                    (const_int 4 [0x4])) 2))) 414 {movsicc+2} (nil)
    (expr_list:REG_DEAD (reg/v:SI 0 %eax)
        (nil)))

reg22 does not get a hard register, so instead it lives in a stack slot.

We end up creating reloads for the references to reg22:

Reload 0: reload_in (SI) = (reg/v:SI 22)
        GENERAL_REGS, RELOAD_FOR_INPUT_ADDRESS (opnum = 3)
        reload_in_reg: (reg/v:SI 22)
Reload 1: reload_in (SI) = (reg/v:SI 22)
        GENERAL_REGS, RELOAD_FOR_INPUT_ADDRESS (opnum = 5)
        reload_in_reg: (reg/v:SI 22)

Then we get into this code in push_reload:


        /* For each non-matching operand that's a MEM or a pseudo-register
           that didn't get a hard register, make an optional reload.
           This may get done even if the insn needs no reloads otherwise.  */

	[ ... ]


So we create an optional reload first mem expression.  Something like this:

Reload 2: reload_in (SI) = (mem/s:SI (plus:SI (reg/v:SI 22)
                                                        (const_int 4 [0x4])) 2)
        GENERAL_REGS, RELOAD_FOR_INPUT (opnum = 3), optional
        reload_in_reg: (mem/s:SI (plus:SI (reg/v:SI 22)
                                                        (const_int 4 [0x4])) 2)

Then we try to create a second optional reload for the same expression and
we get into this code in push_reload:

[ ... ]

  i = find_reusable_reload (&in, out, class, type, opnum, dont_share);

  if (i == n_reloads)
    {
      [ ... ]
    }
  else
    {
	[ ... ]
          /* If we merge reloads for two distinct rtl expressions that
             are identical in content, there might be duplicate address
             reloads.  Remove the extra set now, so that if we later find
             that we can inherit this reload, we can get rid of the
             address reloads altogether.  */
	[ ... ]
   
    }


When creating the second optional reload, we find a reusable reload.  Since
the reusable reload is for a distinct rtl expression that is identical in
content, we end up removing the address replacements for the new reload.

The net result is we never perform the replacement on the second instance
of (mem (plus (reg 22) (const_int 4)).  So after reload we have:

(insn 98 19 32 (set (reg/v:SI 1 %edx)
        (if_then_else:SI (ge:SI (reg/v:SI 0 %eax)
                (mem/s:SI (plus:SI (reg:SI 2 %ecx)
                        (const_int 4 [0x4])) 2))
            (reg/v:SI 1 %edx)
            (mem/s:SI (plus:SI (mem/f:SI (plus:SI (reg:SI 6 %ebp)
                            (const_int 8 [0x8])) 0)
                    (const_int 4 [0x4])) 2))) 414 {movsicc+2} (insn_list/j 118 
(nil))
    (expr_list:REG_DEAD (reg/v:SI 0 %eax)
        (expr_list:REG_DEAD (reg:SI 2 %ecx)
            (nil))))



Which is an invalid insn and cause the x86 port to abort for quite a few
testcases submitted by egcs folks (look at the form of the second mem
expression).

I'm not real familiar with how this code is supposed to work, but clearly
something is broken.  It seems to me you can't simply delete address
replacements like that without (at least) some additional checks for when
it is safe.

A testcase is attached.  Compile with -O2 -march=pentiumpro on an x86 port.
It should fail in gcc-2.95 & the mainline sources.

If you could take a look at this bug it would be greatly appreciated.  Thanks.

typedef struct t_anim_info {
   char        *new_filename;
   long         first_frame_nr; 
} t_anim_info;
static int
p_frames_to_multilayer(t_anim_info *ainfo_ptr,
                      long range_from, long range_to)
{
  long    l_cur_frame_nr;
  long    l_step, l_begin, l_end;
  int  l_tmp_image_id;
  int  l_new_image_id;
  if(range_from > range_to)
  {
    l_step  = -1;      
    if(range_to < ainfo_ptr->first_frame_nr)
    { l_begin = ainfo_ptr->first_frame_nr; 
     }
  }
  else
  {
    l_step  = 1;       
  }
  l_cur_frame_nr = l_begin;
  while(1)
  {
    if(ainfo_ptr->new_filename == ((void *)0) )
    if(l_tmp_image_id < 0)
      gimp_image_delete(l_tmp_image_id);
    if(l_cur_frame_nr == l_end)
       break;
    l_cur_frame_nr += l_step;
  }
}