On Sat, Feb 13, 2016 at 4:45 PM, Francisco Jerez <curroje...@riseup.net> wrote:
> Ilia Mirkin <imir...@alum.mit.edu> writes:
>
>> On Sat, Feb 13, 2016 at 12:01 PM, Serge Martin <edb+m...@sigluy.net> wrote:
>>> ---
>>> src/gallium/state_trackers/clover/core/kernel.cpp | 7 ++++---
>>> 1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/src/gallium/state_trackers/clover/core/kernel.cpp
>>> b/src/gallium/state_trackers/clover/core/kernel.cpp
>>> index 41b3852..a4ef2b1 100644
>>> --- a/src/gallium/state_trackers/clover/core/kernel.cpp
>>> +++ b/src/gallium/state_trackers/clover/core/kernel.cpp
>>> @@ -76,9 +76,10 @@ kernel::launch(command_queue &q,
>>> exec.g_buffers.data(), g_handles.data());
>>>
>>> // Fill information for the launch_grid() call.
>>> - info.block = pad_vector(q, block_size, 1).data(),
>>> - info.grid = pad_vector(q, reduced_grid_size, 1).data(),
>>> - info.pc = find(name_equals(_name), m.sysm).offset;
>>> + memcpy(info.block, pad_vector(q, block_size, 1).data(),
>>> sizeof(info.block));
>>
>> This is equivalent to
>> int *x;
>> {
>> std::vector foo = pad_vector(...);
>> x = foo.data();
>> }
>> use(x)
>>
>> Of course by the time you use x, the underlying vector backing it will
>> have been freed. This was also a problem in the original code before
>> Samuel ever touched it.
>
> Nope, it wasn't, both Serge's and my original code were correct. In C++
> the lifetime of a temporary extends until the end of the topmost
> lexically containing expression (i.e. the end of the memcpy() call in
> Serge's patch and the end of the launch_grid() call in my original
> code), so it's effectively equivalent to:
Huh, I didn't know that. It's either been a *really* long time since
I've done C++, or it changed at some point. Probably the former.
>
> | {
> | int *x;
> | std::vector foo = pad_vector(...);
> | x = foo.data();
> | use(x)
> | }
>
>> You could do something like...
>>
>> {
>> std::vector<uint> padded = pad_vector(...);
>> memcpy(info.block, padded.data(), sizeof(info.block));
>> }
>>
>> and then again for the second one. I couldn't come up with a cleaner
>> alternative... std::copy() needs the begin/end iterators, which would
>> also require a temp var.
>>
>
> That's precisely the reason clover::copy() exists (basically the same as
> std::copy but more easily composable). Serge, can we take v1 of your
> patch and replace memcpy(v, u.data(), size(v)) with copy(u, v) for the
> sake of type safety? With that fixed:
Yeah, at that point you can just do
copy(pad_vector(...), info.grid)
or something along those lines, depending on how the types work out.
>
> Reviewed-by: Francisco Jerez <curroje...@riseup.net>
>
>>> + memcpy(info.grid, pad_vector(q, reduced_grid_size, 1).data(),
>>> +
>>> sizeof(info.grid));
>>> + info.pc = find(name_equals(_name), m.syms).offset;
>>> info.input = exec.input.data();
>>>
>>> q.pipe->launch_grid(q.pipe, &info);
>>> --
>>> 2.5.0
>>>
>>> _______________________________________________
>>> mesa-dev mailing list
>>> mesa-dev@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
