On November 11, 2018 16:36:16 Karol Herbst <[email protected]> wrote:
On Sun, Nov 11, 2018 at 10:48 PM Jason Ekstrand <[email protected]> wrote:
On Sun, Nov 11, 2018 at 3:35 PM Plamena Manolova
<[email protected]> wrote:
Lowering shader variables which depend on the local work group
size being available in nir_lower_system_values is only possible
if the local work group size isn't variable, otherwise this should
be handled by the native driver (if it supports
ARB_compute_variable_group_size).
Signed-off-by: Plamena Manolova <[email protected]>
---
src/compiler/nir/nir_lower_system_values.c | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
diff --git a/src/compiler/nir/nir_lower_system_values.c
b/src/compiler/nir/nir_lower_system_values.c
index bde7eb1180..6fdf9f9c51 100644
--- a/src/compiler/nir/nir_lower_system_values.c
+++ b/src/compiler/nir/nir_lower_system_values.c
@@ -77,6 +77,15 @@ convert_block(nir_block *block, nir_builder *b)
* "The value of gl_GlobalInvocationID is equal to
* gl_WorkGroupID * gl_WorkGroupSize + gl_LocalInvocationID"
*/
+
+ /*
+ * If the local work group size is variable we can't lower the global
+ * invocation id here.
Why not?
+ */
+ if (b->shader->info.cs.local_size_variable) {
I didn't realize we'd added a bit for this. At one point in time, Karol
had patches which had it keyed off of the size being zero. Having a
separate bit is probably fine, it just surpised me.
yeah.. I guess I choose that, because I had nothing better. But I
guess having the size being 0 is good enough as long as we sure it is
0 in relevant cases.
It's fine. I just thought we'd chosen a different convention bit there's
nothing wrong with it. I was just surprised. That's all.
+ break;
+ }
+
nir_ssa_def *group_size = build_local_group_size(b);
nir_ssa_def *group_id = nir_load_work_group_id(b);
nir_ssa_def *local_id = nir_load_local_invocation_id(b);
@@ -115,6 +124,11 @@ convert_block(nir_block *block, nir_builder *b)
}
case SYSTEM_VALUE_LOCAL_GROUP_SIZE: {
+ /* If the local work group size is variable we can't lower it here */
+ if (b->shader->info.cs.local_size_variable) {
+ break;
+ }
+
sysval = build_local_group_size(b);
break;
}
@@ -189,6 +203,14 @@ convert_block(nir_block *block, nir_builder *b)
break;
case SYSTEM_VALUE_GLOBAL_GROUP_SIZE: {
+ /*
+ * If the local work group size is variable we can't lower the global
+ * group size here.
+ */
+ if (b->shader->info.cs.local_size_variable) {
+ break;
+ }
Why can't we lower the global size? It seems like we would want the below
multiplication regardless of whether the local size is constant.
well I am not sure about ARB_compute_variable_group_size, but at least
in CL you know nothing about it at compile time as you specify
everything when you enqueue the kernel. Could be that the number of
work_groups is fixed with ARB_compute_variable_group_size though
Why can't you multiply two non-constant things together? Maybe the driver
can do something now efficient but it's not clear to me that this isn't
what we want.
+
nir_ssa_def *group_size = build_local_group_size(b);
nir_ssa_def *num_work_groups = nir_load_num_work_groups(b);
sysval = nir_imul(b, group_size, num_work_groups);
--
2.11.0
_______________________________________________
mesa-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
