On Mon, 12 Aug 2019 14:30:06 -0600
Jeff Law <l...@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 <joze...@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 <joze...@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 <joze...@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
