This is the mail archive of the
mailing list for the GCC project.
Re: [RFC][PATCH,reload] PR middle-end/42431, fix latent bug in gen_reload_chain_without_interm_reg_p()
- From: Jeff Law <law at redhat dot com>
- To: Peter Bergner <bergner at vnet dot ibm dot com>
- Cc: gcc-patches <gcc-patches at gcc dot gnu dot org>, Vladimir Makarov <vmakarov at redhat dot com>
- Date: Wed, 24 Feb 2010 22:44:36 -0700
- Subject: Re: [RFC][PATCH,reload] PR middle-end/42431, fix latent bug in gen_reload_chain_without_interm_reg_p()
- References: <1267037994.25597.21.camel@otta>
On 02/24/10 11:59, Peter Bergner wrote:
What a mess. When I speak of hunks of reload that need to die, all
the reload scheduling and conflict detection/resolution is high on that
list (probably just after the reload inheritance code). Anyway...
During IRA, pseudos 256 and 270 are marked for spilling and we eventually
call into gen_reload_chain_without_interm_reg_p() with the following two
*) (const:DI (plus:DI (symbol_ref:DI ("*.LANCHOR2") [flags 0x182]) (const_int 16 [0x10])))
*) (const_int 16 [0x10])
As part of its analysis, g_r_c_w_i_r_p() creates a scratch copy of the
larger reload rtx with copy_rtx() so that it can modify it to run some
tests on, before throwing it away. The bug is that g_r_c_w_i_r_p()
expects copy_rtx() to return a true scratch copy we can modify at will and
in any way we wish with no effects on the program's rtl. However, copy_rtx()
doesn't copy rtxs that are sharable, instead it just returns the object
we passed in. In this specific case, we pass in a CONST rtx which is
sharable, so it just returns without making a copy which means we end up
modifying the program's rtl rather than some scratch rtl.
At first, I thought if I added an early exit for reloads that are CONST rtxs
like the patch below, that it would solve the problem:
--- reload1.c (revision 156816)
+++ reload1.c (working copy)
@@ -5221,6 +5221,10 @@ gen_reload_chain_without_interm_reg_p (i
r1 = r2;
r2 = n;
+ if (GET_CODE (rld[r1].in) == CONST)
+ return true;
gcc_assert (reg_mentioned_p (rld[r2].in, rld[r1].in));
regno = rld[r1].regno>= 0 ? rld[r1].regno : rld[r2].regno;
gcc_assert (regno>= 0);
...and it does for this specific test case (actually, it bootstrapped and
regtested on powerpc64-linux with no regressions). However, it doesn't
handle a CONST rtx that is buried within the reload and it doesn't handle
other rtx types that are sharable. Maybe those cases aren't possible?
Does anyone have an opinion on how this should be fixed? Is the patch
above OK, or should we create a copy_rtx_unshared() function that truly
does create a copy of the reload rtx with no sharing? Or a something all
together different type of fix?
I think we can have an embedded const within a mem... ie, the longer
rtx might look like
(plus (mem (const (plus (symbol_ref) (const_int))) (const_int)
Which could lead to the exact same problem we're seeing right now. So I
see the following solutions.
1. Create a true scratch copy.
2. Undo the substitution. This wouldn't be that hard. We just
accumulate a record of the substitutions made by substitute and use
those records to undo the substitution.
3. Find some way to rethink the code not to need the substitution.
I didn't come up with anything clean for #3.
#1 is very easy, but will generate more garbage RTL which we can't free
until reload is complete. Depending on how often these checks are being
performed, it could be a substantial memory leak for the duration of reload.
#2 is relatively simple I think. I think recording in substitute is
safer than just replacing references to the reload reg with the original
value of rld.in, just in case the reload reg is used for some other
purpose in the insn.