On Sun, 2014-10-12 at 01:56 +0300, Francisco Jerez wrote: > Jan Vesely <jan.ves...@rutgers.edu> writes: > > > On Sat, 2014-10-11 at 12:47 +0300, Francisco Jerez wrote: > >> Jan Vesely <jan.ves...@rutgers.edu> writes: > >> > >> > On Wed, 2014-10-08 at 18:02 +0300, Francisco Jerez wrote: > >> >> Jan Vesely <jan.ves...@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.ves...@rutgers.edu> > > > > patch3: > > Reviewed-by: Jan Vesely <jan.ves...@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. > > > 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.ves...@rutgers.edu> > >> >> > >> > > >> > -- > >> > Jan Vesely <jan.ves...@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.ves...@rutgers.edu> -- Jan Vesely <jan.ves...@rutgers.edu>
signature.asc
Description: This is a digitally signed message part
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev