svn commit: r190098 - in head/sys/sparc64: fhc sparc64
Christoph Mallon
christoph.mallon at gmx.de
Sat Mar 21 04:03:21 PDT 2009
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 <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.
More information about the svn-src-all
mailing list