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