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:
| {
| 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:
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
signature.asc
Description: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
