From owner-svn-src-head@FreeBSD.ORG Sat Mar 21 11:03:21 2009 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 60C0E1065670 for ; Sat, 21 Mar 2009 11:03:21 +0000 (UTC) (envelope-from christoph.mallon@gmx.de) Received: from mail.gmx.net (mail.gmx.net [213.165.64.20]) by mx1.freebsd.org (Postfix) with SMTP id D480C8FC39 for ; Sat, 21 Mar 2009 11:03:19 +0000 (UTC) (envelope-from christoph.mallon@gmx.de) Received: (qmail invoked by alias); 21 Mar 2009 11:03:17 -0000 Received: from p54A3DBD1.dip.t-dialin.net (EHLO tron.homeunix.org) [84.163.219.209] by mail.gmx.net (mp064) with SMTP; 21 Mar 2009 12:03:17 +0100 X-Authenticated: #1673122 X-Provags-ID: V01U2FsdGVkX185NZM6mIM5MXGxgzHOpzInVIwoe0t0sQB/X0XaKE T9uG/uYuemZObQ Message-ID: <49C4C974.5050209@gmx.de> Date: Sat, 21 Mar 2009 12:03:16 +0100 From: Christoph Mallon User-Agent: Thunderbird 2.0.0.19 (X11/20090103) MIME-Version: 1.0 To: Marius Strobl References: <200903192029.n2JKTN54074599@svn.freebsd.org> In-Reply-To: <200903192029.n2JKTN54074599@svn.freebsd.org> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Y-GMX-Trusted: 0 X-FuHaFi: 0.4 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 X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 21 Mar 2009 11:03:21 -0000 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. 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! 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. Here's a very small example: #include 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.