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

[Jan Vesely]

On Sun, 2014-10-12 at 01:56 +0300, Francisco Jerez wrote:
> 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.

I think you forgot break; statements during the conversion.
I tried enabling the errors/warning globally but it exploded in multiple
places. I'll see if it can be used only for clover

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

-- 
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/53b1ee43/attachment.sig>