add h8sx support to h8300
Richard Sandiford
rsandifo@redhat.com
Mon Jul 12 20:14:00 GMT 2004
Alexandre Oliva <aoliva@redhat.com> writes:
> Maybe you can still get the problem with dg/builtin-apply2.c? I
> remember it failed to compile because of some movmd-related problems.
Thanks for the pointer. builtin-apply2.c does indeed fail without
your change.
It turns out that the problem was in:
if (!regs_ever_live[FP_REG])
return NO_REGS;
When I wrote the code, there was no such thing as HFP_REG, and FP_REG was 6.
As explained (poorly, I expect ;) in the comment above the function, the idea
was to return NO_REGS if er6 wasn't yet live. So with the new HFP_REG/FP_REG
distinction, the code should instead read:
if (!regs_ever_live[HFP_REG])
return NO_REGS;
FWIW, the patch below puts back the old code, but with the additional
FP_REG->HFP_REG change, and builtin-apply2.c now compiles.
> Anyhow, whether the testcase passes with your patch or not is not very
> relevant. Using !D at that point is wrong, for the reasons explained
> in the comments.
>
> ! prevents reload from even considering an alternative. So, given
> `d,!D', if it couldn't satisfy the constraint in local or global
> alloc, it will require r6 (or, worse, NO_REGS, if `d' happens to
> expand differently) during reload. NO_REGS will obviously be
> impossible to satisfy, and r6 may be impossible to satisfy if the
> frame pointer is found to be needed during reload (this happens in
> builtin-apply2).
See above for the builtin-apply2 bit. But wrt the first sentence,
the idea is precisely to prevent reload from considering the 'D'
alternative if er6 is an allocatable register. I.e., when the
instruction is being reloaded, there are two cases:
(a) er6 is allocatable. Then:
'd' maps to DESTINATION_REGS
'D' maps to GENERAL_REGS
but we absolutely want reload to pick the first alternative
if it can, even if that means spilling into and out of er6.
If it doesn't choose the first alternative, we have to spill
er6 ourselves anyway.
(b) er6 is not allocatable. Then:
'd' maps to NO_REGS
'D' maps to GENERAL_REGS
Reload will ignore the first alternative since, like you say,
NO_REGS can't be satisfied. It is forced to use the second
alternative instead.
The built-in-setjmp.c -O3 -fomit-frame-pointer failure (which, to remind
anyone else reading, is there with and without the patch below) does show
up a problem. And this might well be the problem you were trying to explain
above. (Sorry if so! I couldn't quite follow what you were saying.)
We have the following sequence of events, all in the main reload() loop:
1. We pick a set of reloads each instruction. In the case
of movmd, we pick DESTINATION_REGS (the class containing
only the frame pointer), since the frame pointer is still
allocatable at this point.
2. We discover that the function now needs a frame pointer.
3. We discover that the frame pointer was previously needed
as a spill register and that we must therefore recompute
the reloads (something_changed = 1).
4. We nevertheless proceed to select_reload_regs() for the old set
of reloads. This leads to the movmd spill failure since there
are no longer any allocatable registers in DESTINATION_REGS.
This probably shows my ignorance, but I wasn't planning on (4). I was
expecting the reload loop to repeat straight away once it realised that
the old reloads weren't viable. So it seems you just can't have a
register class that includes only the frame pointer.
As a proof of concept:
Index: reload1.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/reload1.c,v
retrieving revision 1.441
diff -c -p -F^\([(a-zA-Z0-9_]\|#define\) -r1.441 reload1.c
*** reload1.c 9 Jul 2004 03:29:34 -0000 1.441
--- reload1.c 12 Jul 2004 13:32:01 -0000
*************** reload (rtx first, int global)
*** 1007,1012 ****
--- 1007,1014 ----
something_changed = 1;
}
}
+ if (something_changed)
+ continue;
select_reload_regs ();
if (failure)
bypasses select_reload_regs() when the set of eliminable registers
has changed. It fixes the testcase, but I doubt it's right. ;)
Richard
Index: config/h8300/h8300.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/config/h8300/h8300.c,v
retrieving revision 1.290
diff -c -p -F^\([(a-zA-Z0-9_]\|#define\) -r1.290 h8300.c
*** config/h8300/h8300.c 8 Jul 2004 03:40:31 -0000 1.290
--- config/h8300/h8300.c 9 Jul 2004 09:34:24 -0000
*************** h8300_init_once (void)
*** 454,469 ****
before reload so that register allocator will pick the second
alternative.
! - we would like 'D' to be be NO_REGS when the frame pointer isn't
! live, but we the frame pointer may turn out to be needed after
! we start reload, and then we may have already decided we don't
! have a choice, so we can't do that. Forcing the register
! allocator to use er6 if possible might produce better code for
! small functions: it's more efficient to save and restore er6 in
! the prologue & epilogue than to do it in a define_split.
! Hopefully disparaging 'D' will have a similar effect, without
! forcing a reload failure if the frame pointer is found to be
! needed too late. */
enum reg_class
h8300_reg_class_from_letter (int c)
--- 454,465 ----
before reload so that register allocator will pick the second
alternative.
! - 'D' should be NO_REGS when the frame pointer isn't live.
! The idea is to *make* it live by restricting the register allocator
! to the first alternative. This isn't needed for correctness
! but it produces better code for small functions: it's more
! efficient to save and restore er6 in the prologue & epilogue
! than to do it in a define_split. */
enum reg_class
h8300_reg_class_from_letter (int c)
*************** h8300_reg_class_from_letter (int c)
*** 484,491 ****
return DESTINATION_REGS;
case 'D':
! /* The meaning of a constraint shouldn't change dynamically, so
! we can't make this NO_REGS. */
return GENERAL_REGS;
case 'f':
--- 480,487 ----
return DESTINATION_REGS;
case 'D':
! if (!regs_ever_live[HFP_REG])
! return NO_REGS;
return GENERAL_REGS;
case 'f':
*************** h8300_swap_into_er6 (rtx addr)
*** 3051,3060 ****
{
push (HARD_FRAME_POINTER_REGNUM);
emit_move_insn (hard_frame_pointer_rtx, addr);
- if (REGNO (addr) == SP_REG)
- emit_move_insn (hard_frame_pointer_rtx,
- plus_constant (hard_frame_pointer_rtx,
- GET_MODE_SIZE (word_mode)));
}
/* Move the current value of er6 into ADDR and pop its old value
--- 3047,3052 ----
*************** h8300_swap_into_er6 (rtx addr)
*** 3063,3070 ****
void
h8300_swap_out_of_er6 (rtx addr)
{
! if (REGNO (addr) != SP_REG)
! emit_move_insn (addr, hard_frame_pointer_rtx);
pop (HARD_FRAME_POINTER_REGNUM);
}
--- 3055,3061 ----
void
h8300_swap_out_of_er6 (rtx addr)
{
! emit_move_insn (addr, hard_frame_pointer_rtx);
pop (HARD_FRAME_POINTER_REGNUM);
}
Index: config/h8300/h8300.md
===================================================================
RCS file: /cvs/gcc/gcc/gcc/config/h8300/h8300.md,v
retrieving revision 1.286
diff -c -p -F^\([(a-zA-Z0-9_]\|#define\) -r1.286 h8300.md
*** config/h8300/h8300.md 8 Jul 2004 03:40:33 -0000 1.286
--- config/h8300/h8300.md 9 Jul 2004 09:34:25 -0000
*************** (define_insn "movmd_internal_normal"
*** 574,580 ****
(mem:BLK (match_operand:HI 4 "register_operand" "1,1")))
(unspec [(match_operand:HI 5 "register_operand" "2,2")
(match_operand:HI 6 "const_int_operand" "n,n")] UNSPEC_MOVMD)
! (clobber (match_operand:HI 0 "register_operand" "=d,??D"))
(clobber (match_operand:HI 1 "register_operand" "=f,f"))
(set (match_operand:HI 2 "register_operand" "=c,c")
(const_int 0))]
--- 574,580 ----
(mem:BLK (match_operand:HI 4 "register_operand" "1,1")))
(unspec [(match_operand:HI 5 "register_operand" "2,2")
(match_operand:HI 6 "const_int_operand" "n,n")] UNSPEC_MOVMD)
! (clobber (match_operand:HI 0 "register_operand" "=d,!D"))
(clobber (match_operand:HI 1 "register_operand" "=f,f"))
(set (match_operand:HI 2 "register_operand" "=c,c")
(const_int 0))]
*************** (define_insn "movmd_internal"
*** 591,597 ****
(mem:BLK (match_operand:SI 4 "register_operand" "1,1")))
(unspec [(match_operand:HI 5 "register_operand" "2,2")
(match_operand:HI 6 "const_int_operand" "n,n")] UNSPEC_MOVMD)
! (clobber (match_operand:SI 0 "register_operand" "=d,??D"))
(clobber (match_operand:SI 1 "register_operand" "=f,f"))
(set (match_operand:HI 2 "register_operand" "=c,c")
(const_int 0))]
--- 591,597 ----
(mem:BLK (match_operand:SI 4 "register_operand" "1,1")))
(unspec [(match_operand:HI 5 "register_operand" "2,2")
(match_operand:HI 6 "const_int_operand" "n,n")] UNSPEC_MOVMD)
! (clobber (match_operand:SI 0 "register_operand" "=d,!D"))
(clobber (match_operand:SI 1 "register_operand" "=f,f"))
(set (match_operand:HI 2 "register_operand" "=c,c")
(const_int 0))]
*************** (define_insn "stpcpy_internal_normal"
*** 702,708 ****
[(set (mem:BLK (match_operand:HI 3 "register_operand" "0,r"))
(unspec:BLK [(mem:BLK (match_operand:HI 4 "register_operand" "1,1"))]
UNSPEC_STPCPY))
! (clobber (match_operand:HI 0 "register_operand" "=d,??D"))
(clobber (match_operand:HI 1 "register_operand" "=f,f"))
(clobber (match_operand:HI 2 "register_operand" "=c,c"))]
"TARGET_H8300SX && TARGET_NORMAL_MODE"
--- 702,708 ----
[(set (mem:BLK (match_operand:HI 3 "register_operand" "0,r"))
(unspec:BLK [(mem:BLK (match_operand:HI 4 "register_operand" "1,1"))]
UNSPEC_STPCPY))
! (clobber (match_operand:HI 0 "register_operand" "=d,!D"))
(clobber (match_operand:HI 1 "register_operand" "=f,f"))
(clobber (match_operand:HI 2 "register_operand" "=c,c"))]
"TARGET_H8300SX && TARGET_NORMAL_MODE"
*************** (define_insn "stpcpy_internal"
*** 716,722 ****
[(set (mem:BLK (match_operand:SI 3 "register_operand" "0,r"))
(unspec:BLK [(mem:BLK (match_operand:SI 4 "register_operand" "1,1"))]
UNSPEC_STPCPY))
! (clobber (match_operand:SI 0 "register_operand" "=d,??D"))
(clobber (match_operand:SI 1 "register_operand" "=f,f"))
(clobber (match_operand:SI 2 "register_operand" "=c,c"))]
"TARGET_H8300SX && !TARGET_NORMAL_MODE"
--- 716,722 ----
[(set (mem:BLK (match_operand:SI 3 "register_operand" "0,r"))
(unspec:BLK [(mem:BLK (match_operand:SI 4 "register_operand" "1,1"))]
UNSPEC_STPCPY))
! (clobber (match_operand:SI 0 "register_operand" "=d,!D"))
(clobber (match_operand:SI 1 "register_operand" "=f,f"))
(clobber (match_operand:SI 2 "register_operand" "=c,c"))]
"TARGET_H8300SX && !TARGET_NORMAL_MODE"
More information about the Gcc-patches
mailing list