[Mesa-dev] [PATCH 0/3] cl workdim v2

[Francisco Jerez]

Jan Vesely <jan.vesely at rutgers.edu> writes:

> On Sat, 2014-10-11 at 12:47 +0300, Francisco Jerez wrote:
>> Jan Vesely <jan.vesely at rutgers.edu> writes:
>> 
>> > On Wed, 2014-10-08 at 18:02 +0300, Francisco Jerez wrote:
>> >> Jan Vesely <jan.vesely at rutgers.edu> writes:
>> >> 
>> >> > [SNIP]
>> >> >> > 
>> >> >> > > I also don't like that this way there is no difference between
>> >> >> > > explicit and implicit kernel arguments. On the other hand it's simple,
>> >> >> > > and does not need additional per driver code.
>> >> >> > >
>> >> >> > Yeah...  We definitely want to hide these from the user, as e.g. the
>> >> >> > CL_KERNEL_NUM_ARGS param is required by the spec to return the number of
>> >> >> > arguments provided by the user, and we don't want the user to set
>> >> >> > implicit args, so it gets a bit messy.  I think I like better your
>> >> >> > original idea of passing them as launch_grid() arguments, even though
>> >> >> > the grid offset and dimension parameters are somewhat artificial from a
>> >> >> > the hardware's point of view.
>> >> >> 
>> >> >> sorry to bug you some more with this. I tried one more thing before
>> >> >> going back to the launch_grid parameters. this time it implements a
>> >> >> parallel infrastructure for implicit arguments by creating artificial
>> >> >> module arguments for uint and size_t (I don't think we need more for
>> >> >> implicit arguments).
>> >> >> 
>> >> >> I only added the work dimension argument but adding more should be easy.
>> >> >> If you think that the launch_grid way is better, I'll stop experimenting
>> >> >> as I ran out of ideas I wanted to try.
>> >> >
>> >> > ping
>> >> > should I just resend using git instead of attachments?
>> >> 
>> >> Hi Jan, I'm sorry, I finally had a while to have a look into this.  I've
>> >> taken your series and tried to fix the couple of issues I wasn't very
>> >> comfortable with, see the attached series.  Does it look OK to you?
>> >> Note that it's completely untested, maybe you could give it a run on
>> >> your system?
>> >
>> > Hi,
>> >
>> > It took me a while to get back to this too.
>> >
>> > the first patch is kind of unrelated and imo can go in independently
>> > (you can add my R-b).
>> >
>> Thanks, just pushed it with your R-b.
>> 
>> > I'll need to spend some more time (hopefully this weekend) to fully
>> > understand the rest and give it a R-b (if you need/want it).
>> 
>> Please do.
>
> patch2 with the added fix: 
> Reviewed-by: Jan Vesely <jan.vesely at rutgers.edu>
>
> patch3:
> Reviewed-by: Jan Vesely <jan.vesely at rutgers.edu>

Thanks, I've pushed the series with your R-b.

> just a question
> is there a reason to use series of ifs instead of a switch statement?
> I mean if we used switches we can use -Werror=switch for compile time
> detection of missing cases (or -Werror=switch-enum if we want to keep
> the default case)
>
None in this case other than I tend to avoid switch-case statements
instinctively for some reason.  But detecting missing cases during
compile time sounds good, I've changed it to a switch.

> patch4:
>
>> --- a/src/gallium/state_trackers/clover/llvm/invocation.cpp
>> +++ b/src/gallium/state_trackers/clover/llvm/invocation.cpp
>> @@ -308,6 +308,13 @@ namespace {
>>        bitcode_ostream.flush();
>> 
>>        for (unsigned i = 0; i < kernels.size(); ++i) {
>> +#if HAVE_LLVM < 0x0302
>> +         llvm::TargetData TD(kernel_func->getParent());
>> +#elif HAVE_LLVM < 0x0305
>> +         llvm::DataLayout TD(kernel_func->getParent()->getDataLayout());
>> +#else
>> +         llvm::DataLayout TD(mod);
>> +#endif
>
> These need to be moved below the kernel_func declaration and initialization (just as in the original patch).
> with that fixed LGTM.
>
Uhmm, I wonder how this happened, I must've messed it up at some point
while applying this patch.  Anyway I've fixed it by moving the
initialization of kernel_func and kernel_name to the initializer and
moving the declaration to the top -- IMHO declaring a variable and
leaving it uninitialized until some lines below is bad style unless
there's a good reason to do so.

> regards,
> jan
>
>>           llvm::Function *kernel_func;
>>           std::string kernel_name;
>>           compat::vector<module::argument> args;
>> @@ -318,14 +325,6 @@ namespace {
>>           for (llvm::Function::arg_iterator I = kernel_func->arg_begin(),
>>                                        E = kernel_func->arg_end(); I != E; ++I) {
>>              llvm::Argument &arg = *I;
>> -#if HAVE_LLVM < 0x0302
>> -            llvm::TargetData TD(kernel_func->getParent());
>> -#elif HAVE_LLVM < 0x0305
>> -            llvm::DataLayout TD(kernel_func->getParent()->getDataLayout());
>> -#else
>> -            llvm::DataLayout TD(mod);
>> -#endif
>> -
>>              llvm::Type *arg_type = arg.getType();
>>              const unsigned arg_store_size = TD.getTypeStoreSize(arg_type);
>
>
>
>> 
>> > but it works (with the same changes to llvm and libclc as my patches
>> > need), with the attached fix.
>> 
>> Oh, good catch, thanks.
>> 
>> > so with that change you can add my acked/tested by.
>> > I ran a full piglit with no changes compared to my version
>> >
>> > regards,
>> > Jan
>> >
>> >
>> >> 
>> >> Thanks.
>> >> 
>> >> >
>> >> >> 
>> >> >> thanks,
>> >> >> jan
>> >> >
>> >> > [SNIP]
>> >> >
>> >> > -- 
>> >> > Jan Vesely <jan.vesely at rutgers.edu>
>> >> 
>> >
>> > -- 
>> > Jan Vesely <jan.vesely at rutgers.edu>
>> > diff --git a/src/gallium/state_trackers/clover/core/module.hpp b/src/gallium/state_trackers/clover/core/module.hpp
>> > index 268e3ba..ee6caf9 100644
>> > --- a/src/gallium/state_trackers/clover/core/module.hpp
>> > +++ b/src/gallium/state_trackers/clover/core/module.hpp
>> > @@ -80,7 +80,7 @@ namespace clover {
>> >                    enum semantic semantic = general) :
>> >              type(type), size(size),
>> >              target_size(target_size), target_align(target_align),
>> > -            ext_type(ext_type), semantic(general) { }
>> > +            ext_type(ext_type), semantic(semantic) { }
>> >  
>> >           argument(enum type type, size_t size) :
>> >              type(type), size(size),
>
> -- 
> Jan Vesely <jan.vesely at rutgers.edu>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20141012/d314faac/attachment.sig>