This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [patch] new portlet: cirrus-maverick
- From: Richard Earnshaw <rearnsha at arm dot com>
- To: Aldy Hernandez <aldyh at redhat dot com>
- Cc: rearnsha at arm dot com, nickc at redhat dot com, gcc-patches at gcc dot gnu dot org, rsandifo at redhat dot com
- Date: Thu, 15 Aug 2002 12:49:58 +0100
- Subject: Re: [patch] new portlet: cirrus-maverick
- Organization: ARM Ltd.
- Reply-to: Richard dot Earnshaw at arm dot com
Aldy,
I've a number of (hopefully) small niggles with this which I'd like to see
sorted out before it goes in. I'm a little pressed for time at the
moment, but here's a couple for starters.
+ static int is_maverick_insn PARAMS ((rtx));
Please use something like maverick_insn_p() so that
1) it fits in with the convention for predicates.
2) it starts with 'maverick'
+ /* Nonzero if this chip is a Cirrus/DSP. */
+ int arm_is_maverick = 0;
There's much confusion in the ARM backend because these arm_is_xxx
variables are sometimes used to indicate that we are generating code for
the architecture xxx and sometimes that we are trying to tune to processor
xxx. Please can you use arm_tune_maveric or arm_arch_maverick as
appropriate -- we'll try to fix up the other (ab)uses later, but please
let's not propagate the issue further.
- {"arm9e", FL_MODE32 | FL_FAST_MULT | FL_ARCH4 |
FL_LDSCHED },
+ {"armmaverick", FL_MODE32 | FL_FAST_MULT | FL_ARCH4 |
FL_LDSCHED | FL_MAVERICK },
No! the arm9e is a valid ARM processor. Why have you removed it?
+ static int
+ is_load_address (insn)
+ rtx insn;
+ {
+ rtx body, lhs, rhs;;
+
+ if (!insn)
+ return 0;
+
+ if (GET_CODE (insn) != INSN)
+ return 0;
+
+ body = PATTERN (insn);
+
+ if (GET_CODE (body) != SET)
+ return 0;
+
+ lhs = XEXP (body, 0);
+ rhs = XEXP (body, 1);
+
+ return (GET_CODE (lhs) == REG
+ && REGNO_REG_CLASS (REGNO (lhs)) == GENERAL_REGS
+ && (GET_CODE (rhs) == MEM
+ || GET_CODE (rhs) == SYMBOL_REF));
+ }
What is this used for, and why isn't it using the general
constant-reloading code in ARM_MACHINE_DEPENDENT_REORG? I'm worried that
this is going to break somehow.
Also, use single_set(), SET_SRC() and SET_DEST() in this code if you must
keep it.
You should also be using note_invalid_constants(), which will detect moves
of constants into a register that are about to be converted into minipool
loads.
+ is_maverick_insn (insn)
+ rtx insn;
+ {
+ enum attr_maverick attr;
+
+ /* get_attr aborts on USE and CLOBBER. */
+ if (!insn
+ || GET_CODE (insn) != INSN
+ || GET_CODE (PATTERN (insn)) == USE
+ || GET_CODE (PATTERN (insn)) == CLOBBER)
+ return 0;
+
Use active_insn_p(); also use it in maverick_reorg.
for (insn = next_nonnote_insn (first); insn; insn = next_nonnote_insn
(insn))
{
+ if (TARGET_MAVERICK_FIX_INVALID_INSNS
+ && (is_maverick_insn (insn)
+ || GET_CODE (insn) == JUMP_INSN
+ || is_load_address (insn)))
+ maverick_reorg (insn);
+
You should also be using note_invalid_constants(), which will detect
moves of constants into a register that are about to be converted into
minipool loads. Rather than using is_load_address it might be better to
look for insns whose attribue "type" is load etc.
+ %{march=armmaverick:-D__ARM_ARCH_4T__} \
+ %{march=armmaverick:-D__MAVERICK__} \
Exactly which version of the ARM9 is the Maverick based on? The ARM9E
core is a v5TE device.
+ /* Fix invalid Maverick instruction combinations by inserting NOPs. */
+ #define MAVERICK_FIX_INVALID_INSNS (1 << 24)
+
/* Nonzero if the function prologue (and epilogue) should obey
the ARM Procedure Call Standard. */
#define ARM_FLAG_APCS_FRAME (1 << 0)
Please keep these 'bit' assignments in numeric order (so we can easily
tell which have been used).
I think most of the arm.md file changes are OK, but it's sometimes hard to
tell which pattern you are changing. Can you regenerate the diff for that
file with
diff -p -F "^("
R.