This is the mail archive of the mailing list for the GCC project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[PATCH] Bad code due to reload inheritance bug


we're once again seeing a reload problem causing incorrect code
generation (both 3.4 and mainline).  Unfortunately, the bug shows
up only with a proprietary test case, and I was unable to reduce
it (as tends to happen with reload problems :-/).

What happens is the following.  We start out with this (simplified)

# initially, reg 431 is known to hold the value (const_int 9)
# also, reg 609 is set up to hold some value
insn 3019:   (set (reg 610) (reg 431))
insn 1348:   (set (reg 610) (lshift (reg 610) (reg 609)))
insn 1351:   (set (reg 610) (lshift (reg 610) (const_int 16)))
insn 1354:   (set (reg 3) (lshiftrt (reg 610) (const_int 16)))
# finally, the hard reg 3 is used as function argument

Now, during reload, reg 610 does not get assigned a register, so
all these instructions need input/output reloads to access the
corresponding stack slot, in this case 
  (mem (plus (reg 15) (const_int 6240)))
which I'll abbreviate as (mem stack-slot).  I'll also leave out
the additional address reloads required because 6240 is too large
to be used as displacement directly.

For insn 3019, reload decides to reload the const_int 9 into
register 3, and write the output directly to the stack slot:

insn 3475:   (set (reg 3) (const_int 9))     # input reload for 3019
insn 3019:   (set (mem stack-slot) (reg 3))
                                             # optional output reload

For insn 1348, we get an in-out reload using reg 4 as reload register
(note that reg 609 got assigned to reg 1):

insn 3476:   (set (reg 4) (mem stack-slot))  # input reload for 1348
insn 1348:   (set (reg 4) (lshift (reg 4) (reg 1)))
insn 3477:   (set (mem stack-slot) (reg 4))  # output reload for 1348

For insn 1351, we likewise get an in-out reload using reg 4. However,
reload notices it can inherit the value from the previous instruction,
so that it doesn't need an input reload; delete_output_reloads also
sees it can now delete insn 3477.  We get:

insn 3477:   deleted
insn 1351:   (set (reg 4) (lshift (reg 4) (const_int 16)))
insn 3478:   (set (mem stack-slot) (reg 4))  # output reload for 1351

For insn 1354, we again get an in-out reload, this time using reg 3
so that we don't need an output reload.  We can inherit the previous
value by copying from reg 4 to reg 3 (using reload_override_in), and 
eliminate insn 3478 via delete_output_reloads.  We get:

insn 3478:   deleted
insn 3479:   (set (reg 3) (reg 4))           # override_in for 1354
insn 1354:   (set (reg 3) (lshiftrt (reg 3) (const_int 16)))

So far, so good.  However, while processing input reloads for 1354,
there are in fact *two* calls to delete_output_reloads made: one
from emit_input_reload_insns, which deletes the last store from 
reg 4 (i.e. the reload_override_in source) into the reg 610 stack slot 
(i.e. insn 3478), and the second from do_input_reloads, which deletes 
the last store from reg 3 (i.e. the reload register) into the reg 610 
stack slot -- and this turns out to be insn 3019!

Thus insn 3019 is deleted, and we get:

insn 3475:   (set (reg 3) (const_int 9))     # later deleted as dead
insn 3476:   (set (reg 4) (mem stack-slot))  # uninitialized!
insn 1348:   (set (reg 4) (lshift (reg 4) (reg 1)))
insn 1351:   (set (reg 4) (lshift (reg 4) (const_int 16)))
insn 3479:   (set (reg 3) (reg 4))
insn 1354:   (set (reg 3) (lshiftrt (reg 3) (const_int 16)))

which is obviously broken.

Now, one could argue that delete_output_reloads should refuse to
delete insn 3019, because insn 3476 still makes use of the stack slot.
However, it appears that the way delete_output_reloads is set up,
this use is deliberately ignored because it is in itself a reload
from the same pseudo, and is hence supposed to be eliminated via
reload inheritance.

So why does the input reload for 1348 not inherit the value from reg 3?
The problem here is that reload inheritance consults the array
reg_last_reload_reg to determine from what hard reg the last store
to the stack slot for a pseudo was made.  This array in turn is 
updated during *output reload* processing.  Now, in the case of 
insn 3019, there is *no* output reload -- the insn stores to the
stack slot itself.

However, there is also code to catch this situation -- in 
emit_reload_insns, the code block after this interesting comment:

      /* The following if-statement was #if 0'd in 1.34 (or before...).
         It's reenabled in 1.35 because supposedly nothing else
         deals with this problem.  */

This code block in particular checks for the case where an insn
has an optional output reload that was omitted, but the insn itself
stores to the pseudo stack slot.  In this case, it sets up the
reg_last_reload_reg (and associated other arrays) as if there 
actually had been an output reload.

So why doesn't reload inheritance work anyway?  Because after
emit_reload_insns was called from within reload_as_needed,
the routine forget_old_reloads_1 is called to watch out for the
case of an insn storing into any stack slot watched by the
reload inheritance arrays, and clear them out -- unless this 
happens to be just the store that sets up the equivalence in 
the first place:

  /* Since value of X has changed,
     forget any value previously copied from it.  */

  while (nr-- > 0)
    /* But don't forget a copy if this is the output reload
       that establishes the copy's validity.  */
    if (n_reloads == 0 || reg_has_output_reload[regno + nr] == 0)
      reg_last_reload_reg[regno + nr] = 0;

This check uses the reg_has_output_reload array, which is set if
there is an output reload performed.

However, in the special case we're talking about here, we don't actually
*have* an output reload, and thus even though we set up 
reg_last_reload_reg in emit_reload_insns, the field will be cleared
again right away in forget_old_reloads_1.

Thus I propose as fix to this problem that the above-mentioned code
in emit_reload_insns, after setting up reg_last_reload_reg, also sets
reg_has_output_reload to one.  The appended patch implements this.

Bootstrapped/regtested on s390-ibm-linux and s390x-ibm-linux,
fixes my test case.

OK?  (The bug shows up on 3.4 and head, and is probably latent in 3.3).



	* reload1.c (emit_reload_insns): Set reg_has_output_reload to one
	after setting reg_last_reload_reg for optional output reloads.

Index: gcc/reload1.c
RCS file: /cvs/gcc/gcc/gcc/reload1.c,v
retrieving revision 1.432
diff -c -p -r1.432 reload1.c
*** gcc/reload1.c	5 Mar 2004 10:17:40 -0000	1.432
--- gcc/reload1.c	23 Mar 2004 00:26:31 -0000
*************** emit_reload_insns (struct insn_chain *ch
*** 7324,7329 ****
--- 7324,7333 ----
  			CLEAR_HARD_REG_BIT (reg_reloaded_died, src_regno);
  		  reg_last_reload_reg[nregno] = src_reg;
+ 		  /* We have to set reg_has_output_reload here, or else 
+ 		     forget_old_reloads_1 will clear reg_last_reload_reg
+ 		     right away.  */
+ 		  reg_has_output_reload[nregno] = 1;
  Dr. Ulrich Weigand

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]