Re: [PATCH v1 1/1] riscv: Pass RISCVHartArrayState by pointer

[Eric Blake <eblake@redhat.com>]

On 1/17/21 10:52 AM, Philippe Mathieu-Daudé wrote:
> On 1/16/21 11:38 PM, Alistair Francis wrote:
>> On Sat, Jan 16, 2021 at 2:32 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>>
>>> On 1/16/21 12:00 AM, Alistair Francis wrote:
>>>> We were accidently passing RISCVHartArrayState by value instead of

accidentally

>>>> pointer. The type is 824 bytes long so let's correct that and pass it by
>>>> pointer instead.

>>>> -bool riscv_is_32bit(RISCVHartArrayState harts)
>>>> +bool riscv_is_32bit(RISCVHartArrayState *harts)

Definitely better,

>>>>  {
>>>> -    RISCVCPU hart = harts.harts[0];
>>>> +    RISCVCPU hart = harts->harts[0];

but yeah, this still results in a copy (unless the compiler optimizes it).

>>>
>>> This doesn't look improved. Maybe you want:
>>>
>>>        return riscv_cpu_is_32bit(&harts->harts[0].env);

Whereas this is obviously a pointer into the original without relying on
the compiler to elide a copy.

>>
>> I suspect this ends up generating the same code.
> 
> If the compiler is smart enough, but I'm not sure it can figure out
> only 1 element from the structure is accessed...
> My understanding is "first copy the content pointed at '*harts' in
> 'hart' on the stack", then only use "env".
> 
> Cc'ing Eric/Richard to double check.

I agree that relying on the compiler optimization is not as
straightforward as writing the code to directly access the correct
pointer from the get-go.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org