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

[Jan Vesely]

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>
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)

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.

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: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20141011/ec23cb95/attachment.sig>