Date: Sat, 21 Mar 2009 14:03:32 +0100 From: Marius Strobl <marius@alchemy.franken.de> To: Christoph Mallon <christoph.mallon@gmx.de> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r190098 - in head/sys/sparc64: fhc sparc64 Message-ID: <20090321130332.GD67783@alchemy.franken.de> In-Reply-To: <49C4C974.5050209@gmx.de> References: <200903192029.n2JKTN54074599@svn.freebsd.org> <49C4C974.5050209@gmx.de>
next in thread | previous in thread | raw e-mail | index | archive | help
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 **)®); > >- 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20090321130332.GD67783>