Re: [PATCH 22/41] hw/intc/arm_gicv3: Implement GICv4's new redistributor frame

[Peter Maydell]

On Sat, 9 Apr 2022 at 19:32, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 4/8/22 07:15, Peter Maydell wrote:
> > diff --git a/hw/intc/arm_gicv3_redist.c b/hw/intc/arm_gicv3_redist.c
> > index 7c75dd6f072..9f1fe09a78e 100644
> > --- a/hw/intc/arm_gicv3_redist.c
> > +++ b/hw/intc/arm_gicv3_redist.c
> > @@ -442,8 +442,8 @@ MemTxResult gicv3_redist_read(void *opaque, hwaddr 
> > offset, uint64_t *data,
> >        * in the memory map); if so then the GIC has multiple MemoryRegions
> >        * for the redistributors.
> >        */
> > -    cpuidx = region->cpuidx + offset / GICV3_REDIST_SIZE;
> > -    offset %= GICV3_REDIST_SIZE;
> > +    cpuidx = region->cpuidx + offset / gicv3_redist_size(s);
> > +    offset %= gicv3_redist_size(s);
> >
> >       cs = &s->cpu[cpuidx];
> >
> > @@ -501,8 +501,8 @@ MemTxResult gicv3_redist_write(void *opaque, hwaddr 
> > offset, uint64_t data,
> >        * in the memory map); if so then the GIC has multiple MemoryRegions
> >        * for the redistributors.
> >        */
> > -    cpuidx = region->cpuidx + offset / GICV3_REDIST_SIZE;
> > -    offset %= GICV3_REDIST_SIZE;
> > +    cpuidx = region->cpuidx + offset / gicv3_redist_size(s);
> > +    offset %= gicv3_redist_size(s);
>
> In these two cases, it would be better to call the function only once.

The compiler is smart enough to only actually call the function once
(and thus to do the / and % with a single div and imul), so I don't
think that explicitly adding a local variable and doing the call-once
by hand is really worthwhile.

thanks
-- PMM