RFA: Make reg_overlap_mentioned_for_reload_p more exact to avoid bad code generation
Joern Rennecke
joern.rennecke@superh.com
Fri Sep 19 15:06:00 GMT 2003
The problem occurs with the emit order for the reloads of this instruction:
(gdb) call debug_rtx(insn)
(insn 912 909 914 (set (reg/v:SI 246)
(plus:SI (reg/v:SI 223)
(reg/v:SI 224))) 38 {*addsi3_compact} (insn_list 290 (insn_list 380
before merge_assigned_reloads, we have:
Reload 0: reload_in (SI) = (plus:SI (reg/f:SI 14 r14)
(const_int 124 [0x7c]))
GENERAL_REGS, RELOAD_FOR_OUTPUT_ADDRESS (opnum = 0)
reload_in_reg: (plus:SI (reg/f:SI 14 r14)
(const_int 124 [0x7c]))
reload_reg_rtx: (reg:SI 8 r8)
Reload 1: reload_in (SI) = (plus:SI (reg/f:SI 14 r14)
(const_int 64 [0x40]))
GENERAL_REGS, RELOAD_FOR_OTHER_ADDRESS (opnum = 0)
reload_in_reg: (plus:SI (reg/f:SI 14 r14)
(const_int 64 [0x40]))
reload_reg_rtx: (reg:SI 9 r9)
Reload 2: reload_in (SI) = (plus:SI (reg/f:SI 14 r14)
(const_int 64 [0x40]))
GENERAL_REGS, RELOAD_FOR_INPUT_ADDRESS (opnum = 2)
reload_in_reg: (plus:SI (reg/f:SI 14 r14)
(const_int 64 [0x40]))
reload_reg_rtx: (reg:SI 9 r9)
Reload 3: reload_in (SI) = (mem:SI (plus:SI (plus:SI (reg/f:SI 14 r14)
(const_int 64 [0x40
))
(const_int 24 [0x18]))
0 t61 S4 A32])
reload_out (SI) = (mem:SI (plus:SI (plus:SI (reg/f:SI 14 r14)
(const_int 124 [0x7
]))
(const_int 40 [0x28]))
0 t84 S4 A32])
GENERAL_REGS, RELOAD_OTHER (opnum = 0), can't combine
reload_in_reg: (reg/v:SI 223)
reload_out_reg: (reg/v:SI 246)
reload_reg_rtx: (reg:SI 10 r10)
Reload 4: reload_in (SI) = (mem:SI (plus:SI (plus:SI (reg/f:SI 14 r14)
(const_int 64 [0x40
))
(const_int 28 [0x1c]))
0 t62 S4 A32])
GENERAL_REGS, RELOAD_FOR_INPUT (opnum = 2), can't combine
reload_in_reg: (reg/v:SI 224)
reload_reg_rtx: (reg:SI 8 r8)
Note that reload 4 loads an input, while reload 0 loads an output address
for the output part of reload 3. Reloads 4 and 0 use the same reload
register, but that's OK, since they have disjoint lifetimes.
merge_assigned_reloads merges reloads 1 and 2 into a RELOAD_OTHER
reload - that's fine so far - but then it uses
reg_overlap_mentioned_for_reload_p to check if rld[0].in is mentioned in
rld[1].in - i.e. is (plus (fp) (const_int 124)) mentioned in
(plus (fp) (const_int 64)) . When it sees a PLUS for X, it recurses and checks
if (fp) is mentioned in (plus (fp) (const_int 64)), and thus comes up with
a 'positive' result.
So merge_assigned_reload assumes that reload 0 is a secondary reload for
reload 1, and proceeds to change reload 0 into RELOAD_OTHER.
Thus, the lifetime of reload 0 is extended to commence before the one of
reload 3 ends (or even starts, for that matter), yet they still share
the same reload register, so the output address reloaded by reload 0 is
clobbered by the input reload.
The appended patch fixes this problem by avoiding
reg_overlap_mentioned_for_reload_p giving false positives for the PLUSes
that we can expect to see in reload for loading stack-relative addresses.
I've also added handling of MEMs as these might contain similar PLUSes.
bootstrapped on i686-linux-gnu and regression tested on sh-elf.
compared to the published results, I see an additional failure for
i686-linux-gnu in g++.old-deja/g++.mike/net34.C with:
virtual memory exhausted: Invalid argument
. But compiling with --save-temps makes this failure go away, so
this appears to be a genuine resource usage problem (not an infinite
recursion in reload, at any rate).
I haven't added any code to handle the case when there is a genuine
secondary reload, which shares a reload register with another reload
which then becomes conflicting due to the RELOAD_OTHER promotion.
I'm not convinced that it can happen at all, and at any rate I think that
is complex enough to require a testcase to check we get it right.
It should be safe to add a sanity check to make sure this doesn't happen,
though.
I'm copying Toon Moene because AFAICR he expressed an interest in simple
bug fix patches at the gcc summit. reg_overlap_mentioned_for_reload_p
has been fixed recently (in reload terms, on the 22nd January 2002) to handle
PLUS. This returning false positives was deemeed to be conservative then.
However, due to the way the function is used in merge_assigned_reloads,
instead of compiler crashes, we are now silently generating wrong code.
This problem remained undetected for more than a year (although some
might have suffered from it without knowing the cause), till I got
two independent bug reports from different sources in the same week
that I traced down to this reload problem - the one featured here involves
doing reloading for an add instruction, the other involves reloading for a
subtract instruction.
2003-09-02 J"orn Rennecke <joern.rennecke@superh.com>
* reload.c (reg_overlap_mentioned_for_reload_p):
When looking at a PLUS in X, avoid spuriously returning nonzero
when IN is a REG or another simple PLUS, or a MEM containing one.
*** reload.c-dec3 Mon Sep 1 19:49:11 2003
--- reload.c Tue Sep 2 13:52:11 2003
*************** reg_overlap_mentioned_for_reload_p (x, i
*** 6212,6219 ****
|| GET_CODE (x) == CC0)
return reg_mentioned_p (x, in);
else if (GET_CODE (x) == PLUS)
! return (reg_overlap_mentioned_for_reload_p (XEXP (x, 0), in)
! || reg_overlap_mentioned_for_reload_p (XEXP (x, 1), in));
else
abort ();
--- 6212,6233 ----
|| GET_CODE (x) == CC0)
return reg_mentioned_p (x, in);
else if (GET_CODE (x) == PLUS)
! {
! /* We actually want to know if X is mentioned somewhere inside IN.
! We must not say that (plus (sp) (const_int 124)) is in
! (plus (sp) (const_int 64)), since that can lead to incorrect reload
! allocation when spuriously changing a RELOAD_FOR_OUTPUT_ADDRESS
! into a RELOAD_OTHER on behalf of another RELOAD_OTHER. */
! while (GET_CODE (in) == MEM)
! in = XEXP (in, 0);
! if (GET_CODE (in) == REG)
! return 0;
! else if (GET_CODE (in) == PLUS)
! return (reg_overlap_mentioned_for_reload_p (x, XEXP (in, 0))
! || reg_overlap_mentioned_for_reload_p (x, XEXP (in, 1)));
! else return (reg_overlap_mentioned_for_reload_p (XEXP (x, 0), in)
! || reg_overlap_mentioned_for_reload_p (XEXP (x, 1), in));
! }
else
abort ();
More information about the Gcc-patches
mailing list