Ilia Mirkin <imir...@alum.mit.edu> writes:
> 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.
>
Yeah, that's exactly what I had in mind.>> >> 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
