This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [patch] new portlet: cirrus-maverick


> > + static int
> > + is_load_address (insn)
> > +      rtx insn;
> > + {
> > + }
> > 
> > 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.
> 
> This is used in maverick_reorg() to match certain load operations and
> insert a nop inbetween the load and a Maverick instruction.  Like
> this:
> 
> ldr r0, blah
> --> insert nop here
> cfmvsr mvf0, r0
> 

I realize this, but see below.

> This is needed because Cirrus realized too late that they weren't
> handling this in hardware, so they asked for a fixup-phase to clean up
> their mess. 

Blech!

> >     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.
> 
> I don't understand.  All I need is to fix these few instances.  I
> don't care about the rest.  Perhaps I'm missing something.

The point is that until arm_reorg has finished running an insn of the form

	(set (reg:SI ) (const_int NOT_VALID_FOR_MOV))

will exist (in fact, any constant expression); a symbol_ref is just a 
special case of this.  arm_reorg finds these and converts them into 
mini-pool slots, eg

	(set (reg:SI) (mem:SI(label_in_code)))

and these then get emitted as 
	ldr	Rd, LX+offset
	...
	b	somewhere
LX:
	.word
	.word
	...

You need to find and fix all these invalid-constant mov insns as well if 
your code is to be robust -- note_invalid_constants() will tell you if the 
insn contains such an expression.

> > Exactly which version of the ARM9 is the Maverick based on?  The ARM9E 
> > core is a v5TE device.
> 
> Arm920T.

OK, that's definitely a v4T device.

> + /* Return nonzero if INSN is an LDR R0,ADDR instruction.  */
> + 
> + 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 = SET_DEST (body);
> +   rhs = SET_SRC (body);
> + 
> +   return (GET_CODE (lhs) == REG
> + 	  && REGNO_REG_CLASS (REGNO (lhs)) == GENERAL_REGS
> + 	  && (GET_CODE (rhs) == MEM
> + 	      || GET_CODE (rhs) == SYMBOL_REF));
> + }

I forget precisely when we blat SUBREG expressions.  You might need to 
handle that here as well.  Consider

	(set (subreg:SI (reg:SF 0 "r0") 0) (const_int 0x80000000))

Also, if we are keeping this, please rename it to arm_memory_load_p()


arm.h:   

> + /* Nonzero if this chip is a Maverick variant.  */
> + extern int arm_is_maverick;
> + 

You missed this in the cleanup!

   
> --- 700,706 ----
>   
>   /* Define this if most significant word of doubles is the lowest numbered.
>      This is always true, even when in little-endian mode.  */
> ! #define FLOAT_WORDS_BIG_ENDIAN (!TARGET_MAVERICK || WORDS_BIG_ENDIAN)
>   

Please update the comment.

> *************** enum reg_class
> *** 1042,1047 ****
> --- 1098,1104 ----
>   {					\
>     { 0x0000000 }, /* NO_REGS  */		\
>     { 0x0FF0000 }, /* FPU_REGS */		\
> +   { 0xf8000000, 0x000007FF }, /* MAVERICK_REGS */	\
>     { 0x00000FF }, /* LO_REGS */		\
>     { 0x0002000 }, /* STACK_REG */	\
>     { 0x00020FF }, /* BASE_REGS */	\
Can you pad this out so that all entries have 2 words.

> Index: doc/invoke.texi
> ===================================================================
> RCS file: /cvs/uberbaum/gcc/doc/invoke.texi,v
> retrieving revision 1.170
> diff -c -p -r1.170 invoke.texi
> *** doc/invoke.texi	11 Aug 2002 09:47:47 -0000	1.170
> --- doc/invoke.texi	15 Aug 2002 18:08:17 -0000
> *************** in the following sections.

Don't you need to add armmaverick to the list of supported -mcpu= types?

> Index: config/arm/arm.md
> ===================================================================
> RCS file: /cvs/uberbaum/gcc/config/arm/arm.md,v
> retrieving revision 1.104
> diff -p -F^( -r1.104 arm.md
> *** config/arm/arm.md	29 Jul 2002 12:41:46 -0000	1.104
> --- config/arm/arm.md	15 Aug 2002 18:09:30 -0000
> *************** (define_attr "shift" "" (const_int 0))
> *** 123,128 ****
> --- 123,154 ----
>   ; performance we should try and group them together).
>   (define_attr "fpu" "fpa,fpe2,fpe3" (const (symbol_ref "arm_fpu_attr")))
>   
> + (define_attr "maverick_fpu" "fpa,fpe2,fpe3,yes" (const (symbol_ref "arm_fpu_attr")))

Why have you created a macro which is a superset of "fpu"? In particular 
since it uses the same symbol as "fpu" this means that "fpu" can now hold 
an illegal value.  You should define "fpu" to be "fpa,fpe2,fpe3,maverick", 
and then if you really need a maverick_fpu attribute (I'm not sure you do) 
you can define it as
  (define_attr "maverick_fpu" "no,yes"
	(if_then_else (eq_attr "fpu" "maverick")
	 (const_string "yes")
	 (const_string "no")))

> + ; Classification of each insn
> + ; farith	Floating point arithmetic (4 cycle)
> + ; dmult		Double multiplies (7 cycle)
> + (define_attr "maverick_type" "normal,farith,dmult" (const_string "normal"))

Again, can you not just extend the existing "type" attribute with the 
additional maverick classes.

> + (define_attr "maverick" "no,yes,double,compare,move" (const_string "no"))

I don't like the idea of a 5-way decision that has both 'yes' and 'no' as 
two of the alternatives!

> + (define_function_unit "maverick_fpa" 1 0
> +   (and (eq_attr "maverick_fpu" "yes")
> +        (eq_attr "maverick_type" "farith")) 4 1)

Based on the above, I think these should be something like

(define_function_unit "maverick_fpu" 1 0
   (and (eq_attr "fpu" "maverick")
        (eq_attr "type" "mav_farith")) 4 1)
etc.


> + ;; There is no CCFPE or CCFP modes in the code below so we can have
> + ;; one pattern to match either one.  Besides, we're pretty sure we
> + ;; have either CCFPE or CCFP because we made the patterns
> + ;; (arm_gen_compare_reg).

??? This doesn't make sense to me, and it doesn't seem to match the code 
either.



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]