Re: [patch] new portlet: cirrus-maverick

[Richard Earnshaw <rearnsha 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.