On Tue, May 7, 2013 at 4:41 AM, DJ Delorie wrote:
> Index: gcc/cfgexpand.c
> ===================================================================
> --- gcc/cfgexpand.c (revision 198591)
> +++ gcc/cfgexpand.c (working copy)
> @@ -3090,13 +3090,14 @@ expand_debug_expr (tree exp)
> size_t, we need to check for mis-matched modes and correct
> the addend. */
> if (op0 && op1
> && GET_MODE (op0) != VOIDmode && GET_MODE (op1) != VOIDmode
> && GET_MODE (op0) != GET_MODE (op1))
> {
> - if (GET_MODE_BITSIZE (GET_MODE (op0)) < GET_MODE_BITSIZE (GET_MODE
> (op1)))
> + /* Don't try to sign-extend SImode to PSImode, for example. */
> + if (GET_MODE_BITSIZE (GET_MODE (op0)) <= GET_MODE_BITSIZE (GET_MODE
> (op1)))
> op1 = simplify_gen_unary (TRUNCATE, GET_MODE (op0), op1,
> GET_MODE (op1));
> else
> /* We always sign-extend, regardless of the signedness of
> the operand, because the operand is always unsigned
> here even if the original C expression is signed. */
Can't you do just nothing if
GET_MODE_BITSIZE (GET_MODE (op0)) == GET_MODE_BITSIZE (GET_MODE (op1))?
> Index: gcc/simplify-rtx.c
> ===================================================================
> --- gcc/simplify-rtx.c (revision 198591)
> +++ gcc/simplify-rtx.c (working copy)
> @@ -5787,12 +5787,19 @@ simplify_immed_subreg (enum machine_mode
> /* Simplify SUBREG:OUTERMODE(OP:INNERMODE, BYTE)
> Return 0 if no simplifications are possible. */
> rtx
> simplify_subreg (enum machine_mode outermode, rtx op,
> enum machine_mode innermode, unsigned int byte)
> {
> + /* FIXME: hack to allow building of newlib/libc.a for msp430/430x/large
> multilib.
> + The problem is the var-tracking is generating paradoxical SUBREGs. Not
> sure why... */
> + if (!(GET_MODE (op) == innermode
> + || GET_MODE (op) == VOIDmode)
> + || (innermode == VOIDmode)
> + )
> + return NULL_RTX;
IMHO this is not OK, at least not so until the "why" is clarified.
Perhaps Alexandre can have a look, he's with RedHat too isn't he? ;-)
> + (UNS_PROLOGUE_START_MARKER 0)
> + (UNS_PROLOGUE_END_MARKER 1)
> + (UNS_EPILOGUE_START_MARKER 2)
> + (UNS_EPILOGUE_HELPER 3)
> +
> + (UNS_PUSHM 4)
> + (UNS_POPM 5)
> +
> + (UNS_GROW_AND_SWAP 6)
> + (UNS_SWAP_AND_SHRINK 7)
For UNSPECs and UNSPECVs you should use define_c_enum:
(define_c_enum "unspec" [
UNS_PROLOGUE_START_MARKER
UNS_PROLOGUE_END_MARKER
UNS_EPILOGUE_START_MARKER
UNS_EPILOGUE_HELPER
...
])
(define_c_enum "unspecv" [
...
])
> +;; This is nasty. Operand0 is bogus. It is only there so that we can get a
> +;; mode for the %B0 to work. We should use operand1 for this, but that does
> +;; not have a mode.
> +;;
> +;; Operand1 is actually a register, but we cannot accept (REG...) because the
> +;; cprop_hardreg pass can and will renumber registers even inside
> +;; unspec_volatiles. So we take an integer register number parameter and
> +;; fudge it to be a register name when we generate the assembler. We use %I
> +;; because that is the only operator that will omit the # prefix to an
> +;; integer value. Unfortunately it also inverts the integer value, so we
> +;; have pre-invert it when generating this insn. (We could of course add a
> +;; new operator, eg %D, just for this pattern...)
> +;;
> +;; The pushm pattern does not have this problem because of all of the
> +;; frame info cruft attached to it, so cprop_hardreg leaves it alone.
> +(define_insn "popm"
> + [(unspec_volatile [(match_operand 0 "register_operand" "r")
> + (match_operand 1 "immediate_operand" "i")
> + (match_operand 2 "immediate_operand" "i")] UNS_POPM)]
> + ""
> + "POPM%B0\t%2, r%I1"
> + )
I don't understand what the problem is here. If the register is
modified by the post-reload passes, apparently there is a side-effect
in this insn that isn't modeled properly, and the pattern hacks around
the real problem. Is it an auto-inc-dec thing? Is it a SET? Something
CLOBBERED?
> +(define_expand "movsi"
> + [(set (match_operand:SI 0 "nonimmediate_operand" "=rm")
> + (match_operand:SI 1 "general_operand" "rmi"))]
> + ""
> + ""
> + )
What is the define_expand for?
If you need it for some reason, you should remove the constraints,
they have no meaning on a define_expand.
> +(define_insn_and_split "movsi_x"
> + [(set (match_operand:SI 0 "nonimmediate_operand" "=rm")
> + (match_operand:SI 1 "general_operand" "rmi"))]
> + ""
> + "#"
> + "reload_completed"
> + [(set (match_operand:HI 2 "nonimmediate_operand")
> + (match_operand:HI 4 "general_operand"))
> + (set (match_operand:HI 3 "nonimmediate_operand")
> + (match_operand:HI 5 "general_operand"))]
> + "msp430_split_movsi (operands);"
> +)
> +;; This is weird. GCC testcase execute/20020615-1 compiled at -Os
> +;; results in reload producing:
> +;; (set:HI r14) (mem:HI (plus (r14) (const_int 4)))
> +;; (set:HI r15) (mem:HI (plus (r14) (const_int 6)))
> +;; With the first insn corrupting r14 before it is used by the second
> +;; insn. This should not be happening. Reload is generating a:
> +;; (set:SI r14) (mem:SI (plus:PSI (r14) (const_int 4)))
> +;; which is somehow being converted into the two movhi instructions
> +;; above *without* going through the movsi splitter. I have no
> +;; idea how this happens. Any the peephole below is to catch this
> +;; case and reverse the order of the instructions.
> +
> +(define_peephole2
> + [(set (match_operand:HI 0 "register_operand")
> + (mem:HI (plus:PSI (match_operand:PSI 1 "register_operand")
> + (match_operand 2 "immediate_operand"))))
> + (set (match_operand:HI 3 "register_operand")
> + (mem:HI (plus:PSI (match_operand:PSI 4 "register_operand")
> + (match_operand 5 "immediate_operand"))))]
> + " REGNO (operands[0]) == REGNO (operands[4])
> + && REGNO (operands[3]) == REGNO (operands[0]) + 1
> + && REGNO (operands[0]) == REGNO (operands[1])
> + && INTVAL (operands[2]) == INTVAL (operands[5]) - 2"
> + [(set (match_dup 3) (mem:HI (plus:PSI (match_dup 4) (match_dup 5))))
> + (set (match_dup 0) (mem:HI (plus:PSI (match_dup 1) (match_dup 2))))]
> +)
Another hack around a real problem, and a potential source of wrong
code. Not OK IMHO.
> +; NOTE - we are playing a dangerous game with GCC here. We have these two
> +; add patterns and the splitter that follows because our tests have shown
> +; that this results in a significant reduction in code size - because GCC is
> +; able to discard any unused part of the addition. We have to annotate the
> +; patterns with the set and use of the carry flag because otherwise GCC will
> +; discard parts of the addition when they are actually needed. But we have
> +; not annotated all the other patterns that set the CARRY flag as doing so
> +; results in an overall increase in code size[1]. Instead we just *hope*
> +; that GCC will not move a carry-setting instruction in between the first
> +; and second adds.
Scary stuff... :-)
> +;; FIXME: GCC currently (8/feb/2013) cannot handle symbol_refs
> +;; in indirect jumps (cf gcc.c-torture/compile/991213-3.c).
> +(define_insn "indirect_jump"
> + [(set (pc)
> + (match_operand 0 "nonimmediate_operand" "rYl"))]
> + ""
> + "BR%A0\t%0"
> +)
> Index: gcc/config/msp430/t-msp430
> ===================================================================
> --- gcc/config/msp430/t-msp430 (revision 0)
> +++ gcc/config/msp430/t-msp430 (revision 0)
> @@ -0,0 +1,43 @@
> +# Makefile fragment for building GCC for the Renesas RX target.
> +# Copyright (C) 2008, 2009, 2010, 2011 Free Software Foundation, Inc.
Copyright 2012-2013, and not for the RX target.
> +#undef TARGET_HANDLE_OPTION
> +#define TARGET_HANDLE_OPTION msp430_handle_option
Remove the no-op hook and keep the default?
> +#undef TARGET_MS_BITFIELD_LAYOUT_P
> +#define TARGET_MS_BITFIELD_LAYOUT_P msp430_ms_bitfield_layout_p
Likewise.
> +#undef TARGET_FRAME_POINTER_REQUIRED
> +#define TARGET_FRAME_POINTER_REQUIRED msp430_frame_pointer_required
Likewise.
> +#undef TARGET_CAN_ELIMINATE
> +#define TARGET_CAN_ELIMINATE msp430_can_eliminate
Likewise.
> +#undef TARGET_PROMOTE_PROTOTYPES
> +#define TARGET_PROMOTE_PROTOTYPES msp430_promote_prototypes
Likewise.
> +/* We use this to wrap all emitted insns in the prologue. */
> +static rtx
> +F (rtx x)
Perhaps a more descriptive function name? ;-)
> Index: gcc/config/msp430/README.txt
> ===================================================================
> --- gcc/config/msp430/README.txt (revision 0)
> +++ gcc/config/msp430/README.txt (revision 0)
> @@ -0,0 +1,7 @@
> +Random Notes
> +------------
> +
> +The MSP430 port does not use leading underscores. However, the
> +assembler has no way of differentiating between, for example, register
> +R12 and symbol R12. So, if you do "int r12;" in your C program, you
> +may get an assembler error, and will certainly have runtime problems.
Perhaps put this in the user documentation instead (or as well).
Ciao!
Steven
