Re: [Mesa-dev] [PATCH v3 15/19] nir: use load_local_group_size
On March 24, 2018 19:34:05 Rob Clarkwrote: On Sat, Mar 24, 2018 at 8:12 PM, Jason Ekstrand wrote: On March 24, 2018 16:24:57 Rob Clark wrote: On Fri, Mar 23, 2018 at 4:59 PM, Jason Ekstrand wrote: On Fri, Mar 23, 2018 at 1:35 PM, Karol Herbst wrote: On Fri, Mar 23, 2018 at 9:18 PM, Jason Ekstrand wrote: On Fri, Mar 23, 2018 at 12:33 PM, Karol Herbst wrote: From: Rob Clark If local_size is not known at compile time, which is the case with clover, use the load_local_group_size intrinsic instead. Signed-off-by: Karol Herbst --- src/compiler/nir/nir_lower_system_values.c | 25 + 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/src/compiler/nir/nir_lower_system_values.c b/src/compiler/nir/nir_lower_system_values.c index d507c28f421..ff4e09c8e61 100644 --- a/src/compiler/nir/nir_lower_system_values.c +++ b/src/compiler/nir/nir_lower_system_values.c @@ -57,19 +57,28 @@ convert_block(nir_block *block, nir_builder *b) *"The value of gl_GlobalInvocationID is equal to *gl_WorkGroupID * gl_WorkGroupSize + gl_LocalInvocationID" */ + nir_ssa_def *local_size_def; - nir_const_value local_size; - memset(_size, 0, sizeof(local_size)); - local_size.u64[0] = b->shader->info.cs.local_size[0]; - local_size.u64[1] = b->shader->info.cs.local_size[1]; - local_size.u64[2] = b->shader->info.cs.local_size[2]; + /* if local_size[] is already known, use that, otherwise use + * load_local_group_size intrinsic: + */ + if (b->shader->info.cs.local_size[0]) { +nir_const_value local_size; +memset(_size, 0, sizeof(local_size)); +local_size.u64[0] = b->shader->info.cs.local_size[0]; +local_size.u64[1] = b->shader->info.cs.local_size[1]; +local_size.u64[2] = b->shader->info.cs.local_size[2]; + +local_size_def = nir_build_imm(b, 3, bit_size, local_size); + } else { +local_size_def = nir_load_local_group_size(b, bit_size); + } I commented on an earlier patch about how the approach to building the 32/64-bit immediates is wrong. oh right, I totally forgot about that. Setting that aside, this patch looks fine to me in principal. There's a part of me that doesn't like using cs.local_size[0] being the trigger but I think it's probably ok. Maybe we should assert that cs_local_size is either all zero (second case) or all not zero (first case) just to be safe. I think the main problem here is, that even with OpenCL kernels you can specify it, but then overwrite it at runtime again. So yes I agree, that we need something better here. Oh, that's tricky then. We could make nir_lower_system_values take a flag or OpenCL callers could just whack it all to 0 after spirv_to_nir.c. Or you could do recompiles or something. I'm half-way towards thinking we should have MESA_SHADER_KERNEL vs MESA_SHADER_COMPUTE to deal with cl/cuda/etc vs glsl compute shaders.. So far, I've been trying to avoid that, but maybe it would be a better solution.. I think that would be a bit short-sighted. There are cases where that might be a good idea but I doubt this is one of them. It may will be that someone will decide that runtime local sizes would make a great Vulkan extension and then we'd be switching on API for no good reason. hmm, the extension case is a good counter-argument (and probably also applies in the other cases I was thinking of but am too jet-lagged to remember).. I guess until someone comes up with a better idea, sticking to info.cs.local_size[0]==0 => use intrinsic seems sane.. a local_size[0]==0 is impossible. Extra asserts for local_size[1..2]==0 is a good idea Fine by me ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v3 15/19] nir: use load_local_group_size
On Sat, Mar 24, 2018 at 8:12 PM, Jason Ekstrandwrote: > On March 24, 2018 16:24:57 Rob Clark wrote: > > On Fri, Mar 23, 2018 at 4:59 PM, Jason Ekstrand > wrote: > On Fri, Mar 23, 2018 at 1:35 PM, Karol Herbst wrote: > > On Fri, Mar 23, 2018 at 9:18 PM, Jason Ekstrand > wrote: > On Fri, Mar 23, 2018 at 12:33 PM, Karol Herbst > wrote: > > From: Rob Clark > > If local_size is not known at compile time, which is the case with > clover, use the load_local_group_size intrinsic instead. > > Signed-off-by: Karol Herbst > --- > src/compiler/nir/nir_lower_system_values.c | 25 > + > 1 file changed, 17 insertions(+), 8 deletions(-) > > diff --git a/src/compiler/nir/nir_lower_system_values.c > b/src/compiler/nir/nir_lower_system_values.c > index d507c28f421..ff4e09c8e61 100644 > --- a/src/compiler/nir/nir_lower_system_values.c > +++ b/src/compiler/nir/nir_lower_system_values.c > @@ -57,19 +57,28 @@ convert_block(nir_block *block, nir_builder *b) > *"The value of gl_GlobalInvocationID is equal to > *gl_WorkGroupID * gl_WorkGroupSize + > gl_LocalInvocationID" > */ > + nir_ssa_def *local_size_def; > > - nir_const_value local_size; > - memset(_size, 0, sizeof(local_size)); > - local_size.u64[0] = b->shader->info.cs.local_size[0]; > - local_size.u64[1] = b->shader->info.cs.local_size[1]; > - local_size.u64[2] = b->shader->info.cs.local_size[2]; > + /* if local_size[] is already known, use that, otherwise use > + * load_local_group_size intrinsic: > + */ > + if (b->shader->info.cs.local_size[0]) { > +nir_const_value local_size; > +memset(_size, 0, sizeof(local_size)); > +local_size.u64[0] = b->shader->info.cs.local_size[0]; > +local_size.u64[1] = b->shader->info.cs.local_size[1]; > +local_size.u64[2] = b->shader->info.cs.local_size[2]; > + > +local_size_def = nir_build_imm(b, 3, bit_size, > local_size); > > + } else { > +local_size_def = nir_load_local_group_size(b, bit_size); > + } > > > I commented on an earlier patch about how the approach to building the > 32/64-bit immediates is wrong. > > oh right, I totally forgot about that. > > Setting that aside, this patch looks fine to me in principal. There's a > part of me that doesn't like using cs.local_size[0] being the trigger > but I > think it's probably ok. Maybe we should assert that cs_local_size is > either > all zero (second case) or all not zero (first case) just to be safe. > > I think the main problem here is, that even with OpenCL kernels you > can specify it, but then overwrite it at runtime again. So yes I > agree, that we need something better here. > > > Oh, that's tricky then. We could make nir_lower_system_values take a flag > or OpenCL callers could just whack it all to 0 after spirv_to_nir.c. Or you > could do recompiles or something. > > I'm half-way towards thinking we should have MESA_SHADER_KERNEL vs > MESA_SHADER_COMPUTE to deal with cl/cuda/etc vs glsl compute shaders.. > > So far, I've been trying to avoid that, but maybe it would be a better > solution.. > > I think that would be a bit short-sighted. There are cases where that might > be a good idea but I doubt this is one of them. It may will be that someone > will decide that runtime local sizes would make a great Vulkan extension and > then we'd be switching on API for no good reason. > hmm, the extension case is a good counter-argument (and probably also applies in the other cases I was thinking of but am too jet-lagged to remember).. I guess until someone comes up with a better idea, sticking to info.cs.local_size[0]==0 => use intrinsic seems sane.. a local_size[0]==0 is impossible. Extra asserts for local_size[1..2]==0 is a good idea. BR, -R ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v3 15/19] nir: use load_local_group_size
On March 24, 2018 16:24:57 Rob Clarkwrote: On Fri, Mar 23, 2018 at 4:59 PM, Jason Ekstrand wrote: On Fri, Mar 23, 2018 at 1:35 PM, Karol Herbst wrote: On Fri, Mar 23, 2018 at 9:18 PM, Jason Ekstrand wrote: On Fri, Mar 23, 2018 at 12:33 PM, Karol Herbst wrote: From: Rob Clark If local_size is not known at compile time, which is the case with clover, use the load_local_group_size intrinsic instead. Signed-off-by: Karol Herbst --- src/compiler/nir/nir_lower_system_values.c | 25 + 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/src/compiler/nir/nir_lower_system_values.c b/src/compiler/nir/nir_lower_system_values.c index d507c28f421..ff4e09c8e61 100644 --- a/src/compiler/nir/nir_lower_system_values.c +++ b/src/compiler/nir/nir_lower_system_values.c @@ -57,19 +57,28 @@ convert_block(nir_block *block, nir_builder *b) *"The value of gl_GlobalInvocationID is equal to *gl_WorkGroupID * gl_WorkGroupSize + gl_LocalInvocationID" */ + nir_ssa_def *local_size_def; - nir_const_value local_size; - memset(_size, 0, sizeof(local_size)); - local_size.u64[0] = b->shader->info.cs.local_size[0]; - local_size.u64[1] = b->shader->info.cs.local_size[1]; - local_size.u64[2] = b->shader->info.cs.local_size[2]; + /* if local_size[] is already known, use that, otherwise use + * load_local_group_size intrinsic: + */ + if (b->shader->info.cs.local_size[0]) { +nir_const_value local_size; +memset(_size, 0, sizeof(local_size)); +local_size.u64[0] = b->shader->info.cs.local_size[0]; +local_size.u64[1] = b->shader->info.cs.local_size[1]; +local_size.u64[2] = b->shader->info.cs.local_size[2]; + +local_size_def = nir_build_imm(b, 3, bit_size, local_size); + } else { +local_size_def = nir_load_local_group_size(b, bit_size); + } I commented on an earlier patch about how the approach to building the 32/64-bit immediates is wrong. oh right, I totally forgot about that. Setting that aside, this patch looks fine to me in principal. There's a part of me that doesn't like using cs.local_size[0] being the trigger but I think it's probably ok. Maybe we should assert that cs_local_size is either all zero (second case) or all not zero (first case) just to be safe. I think the main problem here is, that even with OpenCL kernels you can specify it, but then overwrite it at runtime again. So yes I agree, that we need something better here. Oh, that's tricky then. We could make nir_lower_system_values take a flag or OpenCL callers could just whack it all to 0 after spirv_to_nir.c. Or you could do recompiles or something. I'm half-way towards thinking we should have MESA_SHADER_KERNEL vs MESA_SHADER_COMPUTE to deal with cl/cuda/etc vs glsl compute shaders.. So far, I've been trying to avoid that, but maybe it would be a better solution.. I think that would be a bit short-sighted. There are cases where that might be a good idea but I doubt this is one of them. It may will be that someone will decide that runtime local sizes would make a great Vulkan extension and then we'd be switching on API for no good reason. --Jason BR, -R I think this looks good for now and we can let OpenCL users of NIR whack it to 0. It's a fairly obvious behavior of "if you don't have it, load it" and we can let the CL driver sort out how they want to handle recompiles. nir_ssa_def *group_id = nir_load_work_group_id(b, bit_size); nir_ssa_def *local_id = nir_load_local_invocation_id(b, bit_size); - sysval = nir_iadd(b, nir_imul(b, group_id, - nir_build_imm(b, 3, bit_size, local_size)), - local_id); + sysval = nir_iadd(b, nir_imul(b, group_id, local_size_def), + local_id); break; } -- 2.14.3 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v3 15/19] nir: use load_local_group_size
On Fri, Mar 23, 2018 at 4:59 PM, Jason Ekstrandwrote: > On Fri, Mar 23, 2018 at 1:35 PM, Karol Herbst wrote: >> >> On Fri, Mar 23, 2018 at 9:18 PM, Jason Ekstrand >> wrote: >> > On Fri, Mar 23, 2018 at 12:33 PM, Karol Herbst >> > wrote: >> >> >> >> From: Rob Clark >> >> >> >> If local_size is not known at compile time, which is the case with >> >> clover, use the load_local_group_size intrinsic instead. >> >> >> >> Signed-off-by: Karol Herbst >> >> --- >> >> src/compiler/nir/nir_lower_system_values.c | 25 >> >> + >> >> 1 file changed, 17 insertions(+), 8 deletions(-) >> >> >> >> diff --git a/src/compiler/nir/nir_lower_system_values.c >> >> b/src/compiler/nir/nir_lower_system_values.c >> >> index d507c28f421..ff4e09c8e61 100644 >> >> --- a/src/compiler/nir/nir_lower_system_values.c >> >> +++ b/src/compiler/nir/nir_lower_system_values.c >> >> @@ -57,19 +57,28 @@ convert_block(nir_block *block, nir_builder *b) >> >>*"The value of gl_GlobalInvocationID is equal to >> >>*gl_WorkGroupID * gl_WorkGroupSize + >> >> gl_LocalInvocationID" >> >>*/ >> >> + nir_ssa_def *local_size_def; >> >> >> >> - nir_const_value local_size; >> >> - memset(_size, 0, sizeof(local_size)); >> >> - local_size.u64[0] = b->shader->info.cs.local_size[0]; >> >> - local_size.u64[1] = b->shader->info.cs.local_size[1]; >> >> - local_size.u64[2] = b->shader->info.cs.local_size[2]; >> >> + /* if local_size[] is already known, use that, otherwise use >> >> + * load_local_group_size intrinsic: >> >> + */ >> >> + if (b->shader->info.cs.local_size[0]) { >> >> +nir_const_value local_size; >> >> +memset(_size, 0, sizeof(local_size)); >> >> +local_size.u64[0] = b->shader->info.cs.local_size[0]; >> >> +local_size.u64[1] = b->shader->info.cs.local_size[1]; >> >> +local_size.u64[2] = b->shader->info.cs.local_size[2]; >> >> + >> >> +local_size_def = nir_build_imm(b, 3, bit_size, >> >> local_size); >> >> >> >> + } else { >> >> +local_size_def = nir_load_local_group_size(b, bit_size); >> >> + } >> > >> > >> > I commented on an earlier patch about how the approach to building the >> > 32/64-bit immediates is wrong. >> > >> >> oh right, I totally forgot about that. >> >> > Setting that aside, this patch looks fine to me in principal. There's a >> > part of me that doesn't like using cs.local_size[0] being the trigger >> > but I >> > think it's probably ok. Maybe we should assert that cs_local_size is >> > either >> > all zero (second case) or all not zero (first case) just to be safe. >> > >> >> I think the main problem here is, that even with OpenCL kernels you >> can specify it, but then overwrite it at runtime again. So yes I >> agree, that we need something better here. > > > Oh, that's tricky then. We could make nir_lower_system_values take a flag > or OpenCL callers could just whack it all to 0 after spirv_to_nir.c. Or you > could do recompiles or something. > I'm half-way towards thinking we should have MESA_SHADER_KERNEL vs MESA_SHADER_COMPUTE to deal with cl/cuda/etc vs glsl compute shaders.. So far, I've been trying to avoid that, but maybe it would be a better solution.. BR, -R > I think this looks good for now and we can let OpenCL users of NIR whack it > to 0. It's a fairly obvious behavior of "if you don't have it, load it" and > we can let the CL driver sort out how they want to handle recompiles. > >> >> >> >> >> >> >> nir_ssa_def *group_id = nir_load_work_group_id(b, bit_size); >> >> nir_ssa_def *local_id = nir_load_local_invocation_id(b, >> >> bit_size); >> >> >> >> - sysval = nir_iadd(b, nir_imul(b, group_id, >> >> - nir_build_imm(b, 3, bit_size, >> >> local_size)), >> >> - local_id); >> >> + sysval = nir_iadd(b, nir_imul(b, group_id, local_size_def), >> >> + local_id); >> >> break; >> >>} >> >> >> >> -- >> >> 2.14.3 >> >> >> >> ___ >> >> mesa-dev mailing list >> >> mesa-dev@lists.freedesktop.org >> >> https://lists.freedesktop.org/mailman/listinfo/mesa-dev >> > >> > > > > > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v3 15/19] nir: use load_local_group_size
On Fri, Mar 23, 2018 at 1:35 PM, Karol Herbstwrote: > On Fri, Mar 23, 2018 at 9:18 PM, Jason Ekstrand > wrote: > > On Fri, Mar 23, 2018 at 12:33 PM, Karol Herbst > wrote: > >> > >> From: Rob Clark > >> > >> If local_size is not known at compile time, which is the case with > >> clover, use the load_local_group_size intrinsic instead. > >> > >> Signed-off-by: Karol Herbst > >> --- > >> src/compiler/nir/nir_lower_system_values.c | 25 > + > >> 1 file changed, 17 insertions(+), 8 deletions(-) > >> > >> diff --git a/src/compiler/nir/nir_lower_system_values.c > >> b/src/compiler/nir/nir_lower_system_values.c > >> index d507c28f421..ff4e09c8e61 100644 > >> --- a/src/compiler/nir/nir_lower_system_values.c > >> +++ b/src/compiler/nir/nir_lower_system_values.c > >> @@ -57,19 +57,28 @@ convert_block(nir_block *block, nir_builder *b) > >>*"The value of gl_GlobalInvocationID is equal to > >>*gl_WorkGroupID * gl_WorkGroupSize + > gl_LocalInvocationID" > >>*/ > >> + nir_ssa_def *local_size_def; > >> > >> - nir_const_value local_size; > >> - memset(_size, 0, sizeof(local_size)); > >> - local_size.u64[0] = b->shader->info.cs.local_size[0]; > >> - local_size.u64[1] = b->shader->info.cs.local_size[1]; > >> - local_size.u64[2] = b->shader->info.cs.local_size[2]; > >> + /* if local_size[] is already known, use that, otherwise use > >> + * load_local_group_size intrinsic: > >> + */ > >> + if (b->shader->info.cs.local_size[0]) { > >> +nir_const_value local_size; > >> +memset(_size, 0, sizeof(local_size)); > >> +local_size.u64[0] = b->shader->info.cs.local_size[0]; > >> +local_size.u64[1] = b->shader->info.cs.local_size[1]; > >> +local_size.u64[2] = b->shader->info.cs.local_size[2]; > >> + > >> +local_size_def = nir_build_imm(b, 3, bit_size, local_size); > >> > >> + } else { > >> +local_size_def = nir_load_local_group_size(b, bit_size); > >> + } > > > > > > I commented on an earlier patch about how the approach to building the > > 32/64-bit immediates is wrong. > > > > oh right, I totally forgot about that. > > > Setting that aside, this patch looks fine to me in principal. There's a > > part of me that doesn't like using cs.local_size[0] being the trigger > but I > > think it's probably ok. Maybe we should assert that cs_local_size is > either > > all zero (second case) or all not zero (first case) just to be safe. > > > > I think the main problem here is, that even with OpenCL kernels you > can specify it, but then overwrite it at runtime again. So yes I > agree, that we need something better here. > Oh, that's tricky then. We could make nir_lower_system_values take a flag or OpenCL callers could just whack it all to 0 after spirv_to_nir.c. Or you could do recompiles or something. I think this looks good for now and we can let OpenCL users of NIR whack it to 0. It's a fairly obvious behavior of "if you don't have it, load it" and we can let the CL driver sort out how they want to handle recompiles. > >> > >> > >> nir_ssa_def *group_id = nir_load_work_group_id(b, bit_size); > >> nir_ssa_def *local_id = nir_load_local_invocation_id(b, > >> bit_size); > >> > >> - sysval = nir_iadd(b, nir_imul(b, group_id, > >> - nir_build_imm(b, 3, bit_size, > >> local_size)), > >> - local_id); > >> + sysval = nir_iadd(b, nir_imul(b, group_id, local_size_def), > >> + local_id); > >> break; > >>} > >> > >> -- > >> 2.14.3 > >> > >> ___ > >> mesa-dev mailing list > >> mesa-dev@lists.freedesktop.org > >> https://lists.freedesktop.org/mailman/listinfo/mesa-dev > > > > > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v3 15/19] nir: use load_local_group_size
On Fri, Mar 23, 2018 at 9:18 PM, Jason Ekstrandwrote: > On Fri, Mar 23, 2018 at 12:33 PM, Karol Herbst wrote: >> >> From: Rob Clark >> >> If local_size is not known at compile time, which is the case with >> clover, use the load_local_group_size intrinsic instead. >> >> Signed-off-by: Karol Herbst >> --- >> src/compiler/nir/nir_lower_system_values.c | 25 + >> 1 file changed, 17 insertions(+), 8 deletions(-) >> >> diff --git a/src/compiler/nir/nir_lower_system_values.c >> b/src/compiler/nir/nir_lower_system_values.c >> index d507c28f421..ff4e09c8e61 100644 >> --- a/src/compiler/nir/nir_lower_system_values.c >> +++ b/src/compiler/nir/nir_lower_system_values.c >> @@ -57,19 +57,28 @@ convert_block(nir_block *block, nir_builder *b) >>*"The value of gl_GlobalInvocationID is equal to >>*gl_WorkGroupID * gl_WorkGroupSize + gl_LocalInvocationID" >>*/ >> + nir_ssa_def *local_size_def; >> >> - nir_const_value local_size; >> - memset(_size, 0, sizeof(local_size)); >> - local_size.u64[0] = b->shader->info.cs.local_size[0]; >> - local_size.u64[1] = b->shader->info.cs.local_size[1]; >> - local_size.u64[2] = b->shader->info.cs.local_size[2]; >> + /* if local_size[] is already known, use that, otherwise use >> + * load_local_group_size intrinsic: >> + */ >> + if (b->shader->info.cs.local_size[0]) { >> +nir_const_value local_size; >> +memset(_size, 0, sizeof(local_size)); >> +local_size.u64[0] = b->shader->info.cs.local_size[0]; >> +local_size.u64[1] = b->shader->info.cs.local_size[1]; >> +local_size.u64[2] = b->shader->info.cs.local_size[2]; >> + >> +local_size_def = nir_build_imm(b, 3, bit_size, local_size); >> >> + } else { >> +local_size_def = nir_load_local_group_size(b, bit_size); >> + } > > > I commented on an earlier patch about how the approach to building the > 32/64-bit immediates is wrong. > oh right, I totally forgot about that. > Setting that aside, this patch looks fine to me in principal. There's a > part of me that doesn't like using cs.local_size[0] being the trigger but I > think it's probably ok. Maybe we should assert that cs_local_size is either > all zero (second case) or all not zero (first case) just to be safe. > I think the main problem here is, that even with OpenCL kernels you can specify it, but then overwrite it at runtime again. So yes I agree, that we need something better here. >> >> >> nir_ssa_def *group_id = nir_load_work_group_id(b, bit_size); >> nir_ssa_def *local_id = nir_load_local_invocation_id(b, >> bit_size); >> >> - sysval = nir_iadd(b, nir_imul(b, group_id, >> - nir_build_imm(b, 3, bit_size, >> local_size)), >> - local_id); >> + sysval = nir_iadd(b, nir_imul(b, group_id, local_size_def), >> + local_id); >> break; >>} >> >> -- >> 2.14.3 >> >> ___ >> mesa-dev mailing list >> mesa-dev@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/mesa-dev > > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v3 15/19] nir: use load_local_group_size
On Fri, Mar 23, 2018 at 12:33 PM, Karol Herbstwrote: > From: Rob Clark > > If local_size is not known at compile time, which is the case with > clover, use the load_local_group_size intrinsic instead. > > Signed-off-by: Karol Herbst > --- > src/compiler/nir/nir_lower_system_values.c | 25 + > 1 file changed, 17 insertions(+), 8 deletions(-) > > diff --git a/src/compiler/nir/nir_lower_system_values.c > b/src/compiler/nir/nir_lower_system_values.c > index d507c28f421..ff4e09c8e61 100644 > --- a/src/compiler/nir/nir_lower_system_values.c > +++ b/src/compiler/nir/nir_lower_system_values.c > @@ -57,19 +57,28 @@ convert_block(nir_block *block, nir_builder *b) >*"The value of gl_GlobalInvocationID is equal to >*gl_WorkGroupID * gl_WorkGroupSize + gl_LocalInvocationID" >*/ > + nir_ssa_def *local_size_def; > > - nir_const_value local_size; > - memset(_size, 0, sizeof(local_size)); > - local_size.u64[0] = b->shader->info.cs.local_size[0]; > - local_size.u64[1] = b->shader->info.cs.local_size[1]; > - local_size.u64[2] = b->shader->info.cs.local_size[2]; > + /* if local_size[] is already known, use that, otherwise use > + * load_local_group_size intrinsic: > + */ > + if (b->shader->info.cs.local_size[0]) { > +nir_const_value local_size; > +memset(_size, 0, sizeof(local_size)); > +local_size.u64[0] = b->shader->info.cs.local_size[0]; > +local_size.u64[1] = b->shader->info.cs.local_size[1]; > +local_size.u64[2] = b->shader->info.cs.local_size[2]; > + > +local_size_def = nir_build_imm(b, 3, bit_size, local_size); + } else { > +local_size_def = nir_load_local_group_size(b, bit_size); > + } > I commented on an earlier patch about how the approach to building the 32/64-bit immediates is wrong. Setting that aside, this patch looks fine to me in principal. There's a part of me that doesn't like using cs.local_size[0] being the trigger but I think it's probably ok. Maybe we should assert that cs_local_size is either all zero (second case) or all not zero (first case) just to be safe. > > nir_ssa_def *group_id = nir_load_work_group_id(b, bit_size); > nir_ssa_def *local_id = nir_load_local_invocation_id(b, > bit_size); > > - sysval = nir_iadd(b, nir_imul(b, group_id, > - nir_build_imm(b, 3, bit_size, > local_size)), > - local_id); > + sysval = nir_iadd(b, nir_imul(b, group_id, local_size_def), > + local_id); > break; >} > > -- > 2.14.3 > > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev