Re: [patch,m68k] Get rid of #ifdef MOTOROLA

[Gunther Nikl <gni at gecko dot de>]

On Fri, Nov 07, 2003 at 08:38:34AM +0100, Bernardo Innocenti wrote:
> As discussed with Gunther Nikl a few weeks days ago, this patch turns
> most occurrences of "#ifdef MOTOROLA" in m68k.c to C statements
> testing for its value.

  Most is not good enough I am afraid. All occurrences must be converted
  and it seems you forgot the MD file. I also believe config.gcc must
  get adjusted since MOTOROLA is already present in some tm_defines.

> Regtesting on a MIT target would be highly desiderable before applying,
> but I have no easy way to do it.

  Since the MD file isn't converted I don't think I can test it.
  I have some comments on the patch.

> diff -u -p -u -p -r1.119 m68k.c
> --- gcc/config/m68k/m68k.c	30 Oct 2003 00:55:15 -0000	1.119
> +++ gcc/config/m68k/m68k.c	7 Nov 2003 07:15:19 -0000
> +#ifndef MOTOROLA
> +#define MOTOROLA 0 /* Use the MIT syntax */
> +#endif

  I don't think this the correct way to define MOTOROLA if its not defined.
  It should be defined in m68k.h to a value used by the majority of targets.
  All other targets must provide the value through config.gcc.

> +/* The ASM_DOT macro allows easy string pasting to handle the differences
> +   between MOTOROLA and MIT syntaxes in asm_fprintf().  */
> +#if MOTOROLA
> +# define ASM_DOT "."
> +# define ASM_DOTW ".w"
> +# define ASM_DOTL ".l"
> +#else
> +# define ASM_DOT ""
> +# define ASM_DOTW ""
> +# define ASM_DOTL ""
> +#endif

  This would have to be in m68k.h then. BTW, you should mention the fact
  that asm_fprintf doesn't handle "%." ;-)

> -#ifdef MOTOROLA
> -	  fprintf (stream, "\tpea (%s)\n\tmove.l %s,%s\n",
> -		   reg_names[FRAME_POINTER_REGNUM],
> -		   reg_names[STACK_POINTER_REGNUM],
> -		   reg_names[FRAME_POINTER_REGNUM]);
> -#else
> -	  fprintf (stream, "\tpea %s@\n\tmovel %s,%s\n",
> -		   reg_names[FRAME_POINTER_REGNUM],
> -		   reg_names[STACK_POINTER_REGNUM],
> -		   reg_names[FRAME_POINTER_REGNUM]);
> -#endif
> -	}
> +	fprintf (stream, MOTOROLA ?
> +			   "\tpea (%s)\n\tmove.l %s,%s\n" :
> +			   "\tpea %s@\n\tmovel %s,%s\n",
> +		 reg_names[FRAME_POINTER_REGNUM],
> +		 reg_names[STACK_POINTER_REGNUM],
> +		 reg_names[FRAME_POINTER_REGNUM]);
>       else if (fsize_with_regs < 0x8000)

  I like this transformation but be careful with argument order.

> -      if (TARGET_COLDFIRE)
> -        {
> -#ifdef MOTOROLA
> -          asm_fprintf (stream, "\t%Omove.l %I%d,%Ra1\n",
> -		       -fsize - current_frame.offset);
> -#else
> -          asm_fprintf (stream, "\tmovel %I%d,%Ra1\n",
> -		       -fsize - current_frame.offset);
> -#endif
> -        }
> -      else
> -        {
> -#ifdef MOTOROLA
> -          asm_fprintf (stream, "\t%Omove.l %I%wd,%Ra1\n", -fsize);
> -#else
> -          asm_fprintf (stream, "\tmovel %I%wd,%Ra1\n", -fsize);
> -#endif
> -        }
> +      asm_fprintf (stream, "\t%Omove" ASM_DOT "l %I%wd,%Ra1\n",
> +	           TARGET_COLDFIRE ? -fsize - current_frame.offset : -fsize);

  This might be wrong because the MOTOROLA case has %O and the non-MOTOROLA
  case doesn't have it. The I would prefer to adjust fsize before the printf
  since fsize is clobbered after that statement:

>       fsize = 0, big = true;
>     }

  here.

  Lastly, it might be a good idea to turn most other symbols used in
  #if[n]def into 0/1 defines eg. SGS, SGS_CMP_ORDER, USE_GAS, HPUX_ASM.
  That might be the next step.

  Gunther