Re: [MSP430][PATCH 1/2] Consolidate handling of hard-coded MCU data

[Jozef Lawrynowicz <jozef dot l at mittosystems dot com>]

On Mon, 12 Aug 2019 14:30:06 -0600
Jeff Law <law@redhat.com> wrote:

> On 8/8/19 6:14 AM, Jozef Lawrynowicz wrote:
> > This patch improves the handling of MCU data by consolidating multiple
> > copies of hard-coded MCU data into a single location, and adds a new function
> > to be used as a single entry point for the extraction of MCU data for the
> > selected MCU.
> > 
> > This ensures the data is only extracted once per invocation of the
> > driver/compiler, whilst previously, the data for the MCU is extracted each time
> > it is needed.
> > 
> > Some notes:
> > - The GNU assembler doesn't do anything with the -mmcu option beyond setting up
> >   the CPU ISA, so if the GCC driver passes it the -mcpu option, which it will
> >   always do if -mmcu is specified, then it is redundant to also pass it -mmcu.
> > - The indenting in some places (e.g. msp430_select_hwmult_lib) looks wrong in
> >   the patched file, but to make the diff a lot easier to read I have kept the
> >   indenting the same as it was before. I can fix this after the patch is
> >   accepted.  
> FWIW, another way to address the indentation issue is with the diff -b
> option which says to ignore whitespace differences.

Ah right, thanks for the tip.

> 
> Regardless, yes, please clean up any indentation issues.
> 
> 
> > 
> > 
> > 0001-MSP430-Devices-1-Consolidate-handling-of-hard-coded-.patch
> > 
> > From cd131b07e0447d104c99317e7ac37c2420c1bf6e Mon Sep 17 00:00:00 2001
> > From: Jozef Lawrynowicz <jozef.l@mittosystems.com>
> > Date: Wed, 31 Jul 2019 22:53:50 +0100
> > Subject: [PATCH 1/2] MSP430: Devices [1]: Consolidate handling of hard-coded
> >  MCU data
> > 
> > gcc/ChangeLog:
> > 
> > 2019-08-XX  Jozef Lawrynowicz  <jozef.l@mittosystems.com>
> > 
> > 	* gcc/config.gcc (msp430*-*-*): Add msp430-devices.o to extra_objs and
> > 	extra_gcc_objs.
> > 	* gcc/config/msp430/driver-msp430.c: Remove msp430_mcu_data.
> > 	(msp430_select_cpu): New spec function.
> > 	(msp430_select_hwmult_lib): Use msp430_extract_mcu_data to extract
> > 	MCU data.
> > 	* gcc/config/msp430/msp430-devices.c: New file.
> > 	* gcc/config/msp430/msp430-devices.h: New file.
> > 	* gcc/config/msp430/msp430.c: Remove msp430_mcu_data.
> > 	(msp430_option_override): Use msp430_extract_mcu_data to extract
> > 	MCU data.
> > 	(msp430_use_f5_series_hwmult): Likewise.
> > 	(use_32bit_hwmult): Likewise.
> > 	(msp430_no_hwmult): Likewise.
> > 	* gcc/config/msp430/msp430.h (ASM_SPEC): Don't pass -mmcu to the
> > 	assembler.
> > 	(DRIVER_SELF_SPECS): Call msp430_select_cpu if -mmcu is used without
> > 	and -mcpu option.
> > 	(EXTRA_SPEC_FUNCTIONS): Add msp430_select_cpu.
> > 	* gcc/config/msp430/t-msp430: Add rule to build msp430-devices.o.
> > 	Remove hard-coded MCU multilib data.
> > 
> > gcc/testsuite/ChangeLog:
> > 
> > 2019-08-XX  Jozef Lawrynowicz  <jozef.l@mittosystems.com>
> > 
> > 	* gcc.target/msp430/msp430.exp
> > 	(check_effective_target_msp430_430_selected): New.
> > 	(check_effective_target_msp430_430x_selected): New.
> > 	(check_effective_target_msp430_mlarge_selected): New.
> > 	(check_effective_target_msp430_hwmul_not_none): New.
> > 	(check_effective_target_msp430_hwmul_not_16bit): New.
> > 	(check_effective_target_msp430_hwmul_not_32bit): New.
> > 	(check_effective_target_msp430_hwmul_not_f5): New.
> > 	(msp430_get_opts): New.
> > 	(msp430_device_permutations_runtest): New.
> > 	* gcc.target/msp430/devices/README: New file.
> > 	* gcc.target/msp430/devices-main.c: New test.
> > 	* gcc.target/msp430/devices/hard-cc430f5123.c: Likewise.
> > 	* gcc.target/msp430/devices/hard-foo.c: Likewise.
> > 	* gcc.target/msp430/devices/hard-msp430afe253.c: Likewise.
> > 	* gcc.target/msp430/devices/hard-msp430cg4616.c: Likewise.
> > 	* gcc.target/msp430/devices/hard-msp430f4783.c: Likewise.
> > 	* gcc.target/msp430/devices/hard-rf430frl154h_rom.c: Likewise.  
> Presumably we aren't supporting switching the selected mcu via
> attributes on a per-function basis?

No that's not supported at the moment.

Given that 430X devices support 430 and 430X ISA instructions, I suppose this is
something that could be added in the future. Similar to (but not as
complicated as), ARM arm/thumb interworking.
 
> 
> > +/* Main entry point to load the MCU data for the given -mmcu into
> > +   extracted_mcu_data.  hard_msp430_mcu_data (initialized at the bottom of this
> > +   file) is searched for the MCU name.
> > +   This function only needs to be executed once, but it can be first called
> > +   from a number of different locations.  */
> > +void
> > +msp430_extract_mcu_data (const char * mcu_name)
> > +{
> > +  static int executed = 0;
> > +  int i;
> > +  if (mcu_name == NULL || executed == 1)
> > +    return;
> > +  executed = 1;
> > +  /* FIXME: This array is alpha sorted - we could use a binary search.  */
> > +  for (i = ARRAY_SIZE (hard_msp430_mcu_data); i--;)
> > +    if (strcasecmp (mcu_name, hard_msp430_mcu_data[i].name) == 0)
> > +      {
> > +	extracted_mcu_data = hard_msp430_mcu_data[i];
> > +	break;
> > +      }  
> I guess we only run this once, so the linear search isn't a serious
> compile-time issue?

True, this is an old FIXME from the original port, but since we are now only
searching this array a maximum of one time now, it's not important.

> 
> Assuming we don't care about switching the selected mcu within a
> compilation unit, OK for the trunk.
> 
> jeff

Great, thanks for the review,
Jozef