svn commit: r190098 - in head/sys/sparc64: fhc sparc64

Marius Strobl marius at alchemy.franken.de
Sat Mar 21 06:03:56 PDT 2009


On Sat, Mar 21, 2009 at 12:03:16PM +0100, Christoph Mallon wrote:
> Hi Marius,
> 
> I think this change makes the code worse from point of the compiler and 
> for people reading it. Please read my explanation below.
> 
> 
> Marius Strobl schrieb:
> >Author: marius
> >Date: Thu Mar 19 20:29:23 2009
> >New Revision: 190098
> >URL: http://svn.freebsd.org/changeset/base/190098
> >
> >Log:
> >  - Failing to register as interrupt controller during attach shouldn't
> >    be fatal so just inform about this instead of panicing.
> >  - Sort device methods.
> >  - Take advantage of KOBJMETHOD_END.
> >  - Remove some redundant variables.
> >
> >Modified:
> >  head/sys/sparc64/fhc/fhc.c
> >  head/sys/sparc64/sparc64/upa.c
> >
> >Modified: head/sys/sparc64/fhc/fhc.c
> >==============================================================================
> >--- head/sys/sparc64/fhc/fhc.c	Thu Mar 19 20:24:30 2009 (r190097)
> >+++ head/sys/sparc64/fhc/fhc.c	Thu Mar 19 20:29:23 2009 (r190098)
> [...]
> >@@ -165,9 +165,7 @@ fhc_attach(device_t dev)
> > 	int central;
> > 	int error;
> > 	int i;
> >-	int nintr;
> >-	int nreg;
> >-	int rid;
> >+	int j;
> > 
> > 	sc = device_get_softc(dev);
> > 	node = ofw_bus_get_node(dev);
> >@@ -177,9 +175,9 @@ fhc_attach(device_t dev)
> > 		central = 1;
> > 
> > 	for (i = 0; i < FHC_NREG; i++) {
> >-		rid = i;
> >+		j = i;
> > 		sc->sc_memres[i] = bus_alloc_resource_any(dev, 
> > 		SYS_RES_MEMORY,
> >-		    &rid, RF_ACTIVE);
> >+		    &j, RF_ACTIVE);
> > 		if (sc->sc_memres[i] == NULL) {
> > 			device_printf(dev, "cannot allocate resource %d\n", 
> > 			i);
> > 			error = ENXIO;
> [...]
> >@@ -259,11 +258,13 @@ fhc_attach(device_t dev)
> > 			 * the IGN and the IGN is constant for all devices
> > 			 * on that FireHose controller.
> > 			 */
> >-			if (intr_controller_register(INTMAP_VEC(sc->sc_ign,
> >+			j = intr_controller_register(INTMAP_VEC(sc->sc_ign,
> > 			    INTINO(bus_read_4(fica->fica_memres, FHC_IMAP))),
> >-			    &fhc_ic, fica) != 0)
> >-				panic("%s: could not register interrupt "
> >-				    "controller for map %d", __func__, i);
> >+			    &fhc_ic, fica);
> >+			if (j != 0)
> >+				device_printf(dev, "could not register "
> >+				    "interrupt controller for map %d (%d)\n",
> >+				    i, j);
> > 		}
> > 	} else {
> > 		snprintf(ledname, sizeof(ledname), "board%d", board);
> >@@ -276,9 +277,9 @@ fhc_attach(device_t dev)
> > 			free(fdi, M_DEVBUF);
> > 			continue;
> > 		}
> >-		nreg = OF_getprop_alloc(child, "reg", sizeof(*reg),
> >+		i = OF_getprop_alloc(child, "reg", sizeof(*reg),
> > 		    (void **)&reg);
> >-		if (nreg == -1) {
> >+		if (i == -1) {
> > 			device_printf(dev, "<%s>: incomplete\n",
> > 			    fdi->fdi_obdinfo.obd_name);
> > 			ofw_bus_gen_destroy_devinfo(&fdi->fdi_obdinfo);
> >@@ -286,19 +287,19 @@ fhc_attach(device_t dev)
> > 			continue;
> > 		}
> > 		resource_list_init(&fdi->fdi_rl);
> >-		for (i = 0; i < nreg; i++)
> >-			resource_list_add(&fdi->fdi_rl, SYS_RES_MEMORY, i,
> >-			    reg[i].sbr_offset, reg[i].sbr_offset +
> >-			    reg[i].sbr_size, reg[i].sbr_size);
> >+		for (j = 0; j < i; j++)
> >+			resource_list_add(&fdi->fdi_rl, SYS_RES_MEMORY, j,
> >+			    reg[j].sbr_offset, reg[j].sbr_offset +
> >+			    reg[j].sbr_size, reg[j].sbr_size);
> > 		free(reg, M_OFWPROP);
> > 		if (central == 1) {
> >-			nintr = OF_getprop_alloc(child, "interrupts",
> >+			i = OF_getprop_alloc(child, "interrupts",
> > 			    sizeof(*intr), (void **)&intr);
> >-			if (nintr != -1) {
> >-				for (i = 0; i < nintr; i++) {
> >-					iv = INTMAP_VEC(sc->sc_ign, intr[i]);
> >+			if (i != -1) {
> >+				for (j = 0; j < i; j++) {
> >+					iv = INTMAP_VEC(sc->sc_ign, intr[j]);
> > 					resource_list_add(&fdi->fdi_rl,
> >-					    SYS_RES_IRQ, i, iv, iv, 1);
> >+					    SYS_RES_IRQ, j, iv, iv, 1);
> > 				}
> > 				free(intr, M_OFWPROP);
> > 			}
> >
> 
> These variables are not redundant. Before the variables had clear names 
> (like rid or nregs, nintr), now they are called i and j, which carries 
> no semantics at all.

IMO the code is simple enough so that the variable names do not
need to carry semantics for clearity, the fact that just two
reused temporary variables are enough for the whole function
underlines this.

> Also j has its address taken AND you use this variable in several 
> different contexts (replacement for rid, later as loop counter). This 
> makes it impossible[0] for the compiler to do many useful optimisations 
> like keeping the value of the variable in a register!

It's good to know that doing so may costs performance in general,
as this is an bus attach function performance totally isn't an
issue here though.

> More local variables cost nothing - especially no stack space, if this 
> is your concern - on any somewhat modern compiler. Use as many of them 
> as you want for code clarity. Do not reuse variables, whose address is 
> taken, because often it is impossible for the compiler to do any 
> optimisations.

My motivation for using fewer local variables is that style(9)
mandates to not define variables in nested scope and to sort
them, so reusing them reduces code churn in the long term.
IIRC you said elsewhere that you're unhappy with at least the
former rule but you should work on getting style(9) then.

> 
> Here's a very small example:
> 
> #include <stdio.h>
> void f(int*);
> void g(void)
> {
>   int i;
>   f(&i); // The address of i escapes here
>   for (i = 0; i != 10; ++i)
>     printf("%d", i);
> }
> 
> The generated code:
> g:
>         subl    $28, %esp
>         leal    24(%esp), %eax // The address of i
>         movl    %eax, (%esp)
>         call    f
>         xorl    %eax, %eax
>         movl    $0, 24(%esp) // write 0 to the spill slot of i
>         .p2align 4,,7
> .L2:
>         movl    %eax, 4(%esp)
>         movl    $.LC0, (%esp)
>         call    printf
>         movl    24(%esp), %eax // i is reloaded here
>         addl    $1, %eax
>         cmpl    $10, %eax
>         movl    %eax, 24(%esp) // i is spilled here
>         jne     .L2
>         addl    $28, %esp
>         ret
> 
> 
> Now lets pass a new variable int j; to f(&j), but still use i for the loop:
> g:
>         pushl   %ebx
>         xorl    %ebx, %ebx // initialise i with 0
>         subl    $24, %esp
>         leal    20(%esp), %eax // The address of j
>         movl    %eax, (%esp)
>         call    f
>         .p2align 4,,7
> .L2:
> 	/* i is held in register %ebx in the whole loop, no spills, no
> 	 * reloads */
>         movl    %ebx, 4(%esp)
>         addl    $1, %ebx
>         movl    $.LC0, (%esp)
>         call    printf
>         cmpl    $10, %ebx
>         jne     .L2
>         addl    $24, %esp
>         popl    %ebx
>         ret
> 
> In short: A variable whose address is not taken is A Good Thing(TM), 
> because the compiler knows "everything" about it and can do many useful 
> optimisations. You can have any number of non-aliased (i.e. address not 
> taken) variables. They cost nothing. If a value carries a specific 
> meaning, use a new local variable with a useful name, this improves the 
> clarity for people who read and try to understand the code.
> The old rule "use as few as possible local variables, because they cost 
> stack space" is dead since many, many years.
> 
> Sorry for the long essay, but I deem it important to tell how compilers 
> work these day.
> 
> Regards
> 	Christoph
> 
> 
> [0] It's impossible because the address is passed to a function, whose 
> body is not known here, so the compiler has to assume the worst, i.e. 
> the address of the variable is stored to a global pointer and the value 
> can be modified at any time by any other function call with unknown body 
> and so it has to reload it in most places where it is used.

Marius



More information about the svn-src-head mailing list