Discussion:
[PATCH 0/2] glsl,radeonsi: interpolateAt* fixes
Add Reply
Nicolai Hähnle
2017-06-16 20:37:13 UTC
Reply
Permalink
Raw Message
Hi all,

Two fixes related to interpolateAt* functions: allowing inputs in interface
blocks, and fixing dynamic array indices in radeonsi.

I've also sent out some piglit tests to exercise these corner cases a bit
more.

Please review!
Thanks,
Nicolai
--
src/compiler/glsl/ast_function.cpp | 7 +-
.../glsl/lower_named_interface_blocks.cpp | 18 ++++
src/gallium/drivers/radeonsi/si_shader.c | 78 +++++++++++++-----
3 files changed, 81 insertions(+), 22 deletions(-)
Nicolai Hähnle
2017-06-16 20:37:15 UTC
Reply
Permalink
Raw Message
From: Nicolai Hähnle <***@amd.com>

The hardware doesn't support it, so we just interpolate all array elements
and then use indirect indexing on the resulting vector.

Clearly, this is not very efficient. There is an argument to be had for
adding if/else, or perhaps even pulling the data out of LDS directly.
Both don't really seem worth the effort, considering that it seems nobody
actually uses this feature.
---
src/gallium/drivers/radeonsi/si_shader.c | 78 ++++++++++++++++++++++++--------
1 file changed, 58 insertions(+), 20 deletions(-)

diff --git a/src/gallium/drivers/radeonsi/si_shader.c b/src/gallium/drivers/radeonsi/si_shader.c
index e525a18..370022f 100644
--- a/src/gallium/drivers/radeonsi/si_shader.c
+++ b/src/gallium/drivers/radeonsi/si_shader.c
@@ -3517,49 +3517,70 @@ static void interp_fetch_args(
}
}

static void build_interp_intrinsic(const struct lp_build_tgsi_action *action,
struct lp_build_tgsi_context *bld_base,
struct lp_build_emit_data *emit_data)
{
struct si_shader_context *ctx = si_shader_context(bld_base);
struct si_shader *shader = ctx->shader;
struct gallivm_state *gallivm = &ctx->gallivm;
+ const struct tgsi_shader_info *info = &shader->selector->info;
LLVMValueRef interp_param;
const struct tgsi_full_instruction *inst = emit_data->inst;
- int input_index = inst->Src[0].Register.Index;
+ const struct tgsi_full_src_register *input = &inst->Src[0];
+ int input_base, input_array_size;
int chan;
int i;
- LLVMValueRef attr_number;
LLVMValueRef params = LLVMGetParam(ctx->main_fn, SI_PARAM_PRIM_MASK);
+ LLVMValueRef array_idx;
int interp_param_idx;
- unsigned interp = shader->selector->info.input_interpolate[input_index];
+ unsigned interp;
unsigned location;

- assert(inst->Src[0].Register.File == TGSI_FILE_INPUT);
+ assert(input->Register.File == TGSI_FILE_INPUT);
+
+ if (input->Register.Indirect) {
+ unsigned array_id = input->Indirect.ArrayID;
+
+ if (array_id) {
+ input_base = info->input_array_first[array_id];
+ input_array_size = info->input_array_last[array_id] - input_base + 1;
+ } else {
+ input_base = inst->Src[0].Register.Index;
+ input_array_size = info->num_inputs - input_base;
+ }
+
+ array_idx = get_indirect_index(ctx, &input->Indirect,
+ input->Register.Index - input_base);
+ } else {
+ input_base = inst->Src[0].Register.Index;
+ input_array_size = 1;
+ array_idx = ctx->i32_0;
+ }
+
+ interp = shader->selector->info.input_interpolate[input_base];

if (inst->Instruction.Opcode == TGSI_OPCODE_INTERP_OFFSET ||
inst->Instruction.Opcode == TGSI_OPCODE_INTERP_SAMPLE)
location = TGSI_INTERPOLATE_LOC_CENTER;
else
location = TGSI_INTERPOLATE_LOC_CENTROID;

interp_param_idx = lookup_interp_param_index(interp, location);
if (interp_param_idx == -1)
return;
else if (interp_param_idx)
interp_param = LLVMGetParam(ctx->main_fn, interp_param_idx);
else
interp_param = NULL;

- attr_number = LLVMConstInt(ctx->i32, input_index, 0);
-
if (inst->Instruction.Opcode == TGSI_OPCODE_INTERP_OFFSET ||
inst->Instruction.Opcode == TGSI_OPCODE_INTERP_SAMPLE) {
LLVMValueRef ij_out[2];
LLVMValueRef ddxy_out = si_llvm_emit_ddxy_interp(bld_base, interp_param);

/*
* take the I then J parameters, and the DDX/Y for it, and
* calculate the IJ inputs for the interpolator.
* temp1 = ddx * offset/sample.x + I;
* interp_param.I = ddy * offset/sample.y + temp1;
@@ -3584,42 +3605,59 @@ static void build_interp_intrinsic(const struct lp_build_tgsi_action *action,

temp1 = LLVMBuildFAdd(gallivm->builder, temp1, interp_el, "");

temp2 = LLVMBuildFMul(gallivm->builder, ddy_el, emit_data->args[1], "");

ij_out[i] = LLVMBuildFAdd(gallivm->builder, temp2, temp1, "");
}
interp_param = lp_build_gather_values(gallivm, ij_out, 2);
}

+ if (interp_param) {
+ interp_param = LLVMBuildBitCast(gallivm->builder,
+ interp_param, LLVMVectorType(ctx->f32, 2), "");
+ }
+
for (chan = 0; chan < 4; chan++) {
LLVMValueRef llvm_chan;
+ LLVMValueRef gather = LLVMGetUndef(LLVMVectorType(ctx->f32, input_array_size));
unsigned schan;

schan = tgsi_util_get_full_src_register_swizzle(&inst->Src[0], chan);
llvm_chan = LLVMConstInt(ctx->i32, schan, 0);

- if (interp_param) {
- interp_param = LLVMBuildBitCast(gallivm->builder,
- interp_param, LLVMVectorType(ctx->f32, 2), "");
- LLVMValueRef i = LLVMBuildExtractElement(
- gallivm->builder, interp_param, ctx->i32_0, "");
- LLVMValueRef j = LLVMBuildExtractElement(
- gallivm->builder, interp_param, ctx->i32_1, "");
- emit_data->output[chan] = ac_build_fs_interp(&ctx->ac,
- llvm_chan, attr_number, params,
- i, j);
- } else {
- emit_data->output[chan] = ac_build_fs_interp_mov(&ctx->ac,
- LLVMConstInt(ctx->i32, 2, 0), /* P0 */
- llvm_chan, attr_number, params);
+ for (unsigned i = 0; i < input_array_size; ++i) {
+ LLVMValueRef attr_number = LLVMConstInt(ctx->i32, input_base + i, false);
+ LLVMValueRef v;
+
+ if (interp_param) {
+ interp_param = LLVMBuildBitCast(gallivm->builder,
+ interp_param, LLVMVectorType(ctx->f32, 2), "");
+ LLVMValueRef i = LLVMBuildExtractElement(
+ gallivm->builder, interp_param, ctx->i32_0, "");
+ LLVMValueRef j = LLVMBuildExtractElement(
+ gallivm->builder, interp_param, ctx->i32_1, "");
+ v = ac_build_fs_interp(&ctx->ac,
+ llvm_chan, attr_number, params,
+ i, j);
+ } else {
+ v = ac_build_fs_interp_mov(&ctx->ac,
+ LLVMConstInt(ctx->i32, 2, 0), /* P0 */
+ llvm_chan, attr_number, params);
+ }
+
+ gather = LLVMBuildInsertElement(gallivm->builder,
+ gather, v, LLVMConstInt(ctx->i32, i, false), "");
}
+
+ emit_data->output[chan] = LLVMBuildExtractElement(
+ gallivm->builder, gather, array_idx, "");
}
}

static LLVMValueRef si_emit_ballot(struct si_shader_context *ctx,
LLVMValueRef value)
{
struct gallivm_state *gallivm = &ctx->gallivm;
LLVMValueRef args[3] = {
value,
ctx->i32_0,
--
2.9.3
Marek Olšák
2017-06-19 19:59:00 UTC
Reply
Permalink
Raw Message
Reviewed-by: Marek Olšák <***@amd.com>

Marek
Post by Nicolai Hähnle
The hardware doesn't support it, so we just interpolate all array elements
and then use indirect indexing on the resulting vector.
Clearly, this is not very efficient. There is an argument to be had for
adding if/else, or perhaps even pulling the data out of LDS directly.
Both don't really seem worth the effort, considering that it seems nobody
actually uses this feature.
---
src/gallium/drivers/radeonsi/si_shader.c | 78 ++++++++++++++++++++++++--------
1 file changed, 58 insertions(+), 20 deletions(-)
diff --git a/src/gallium/drivers/radeonsi/si_shader.c b/src/gallium/drivers/radeonsi/si_shader.c
index e525a18..370022f 100644
--- a/src/gallium/drivers/radeonsi/si_shader.c
+++ b/src/gallium/drivers/radeonsi/si_shader.c
@@ -3517,49 +3517,70 @@ static void interp_fetch_args(
}
}
static void build_interp_intrinsic(const struct lp_build_tgsi_action *action,
struct lp_build_tgsi_context *bld_base,
struct lp_build_emit_data *emit_data)
{
struct si_shader_context *ctx = si_shader_context(bld_base);
struct si_shader *shader = ctx->shader;
struct gallivm_state *gallivm = &ctx->gallivm;
+ const struct tgsi_shader_info *info = &shader->selector->info;
LLVMValueRef interp_param;
const struct tgsi_full_instruction *inst = emit_data->inst;
- int input_index = inst->Src[0].Register.Index;
+ const struct tgsi_full_src_register *input = &inst->Src[0];
+ int input_base, input_array_size;
int chan;
int i;
- LLVMValueRef attr_number;
LLVMValueRef params = LLVMGetParam(ctx->main_fn, SI_PARAM_PRIM_MASK);
+ LLVMValueRef array_idx;
int interp_param_idx;
- unsigned interp = shader->selector->info.input_interpolate[input_index];
+ unsigned interp;
unsigned location;
- assert(inst->Src[0].Register.File == TGSI_FILE_INPUT);
+ assert(input->Register.File == TGSI_FILE_INPUT);
+
+ if (input->Register.Indirect) {
+ unsigned array_id = input->Indirect.ArrayID;
+
+ if (array_id) {
+ input_base = info->input_array_first[array_id];
+ input_array_size = info->input_array_last[array_id] - input_base + 1;
+ } else {
+ input_base = inst->Src[0].Register.Index;
+ input_array_size = info->num_inputs - input_base;
+ }
+
+ array_idx = get_indirect_index(ctx, &input->Indirect,
+ input->Register.Index - input_base);
+ } else {
+ input_base = inst->Src[0].Register.Index;
+ input_array_size = 1;
+ array_idx = ctx->i32_0;
+ }
+
+ interp = shader->selector->info.input_interpolate[input_base];
if (inst->Instruction.Opcode == TGSI_OPCODE_INTERP_OFFSET ||
inst->Instruction.Opcode == TGSI_OPCODE_INTERP_SAMPLE)
location = TGSI_INTERPOLATE_LOC_CENTER;
else
location = TGSI_INTERPOLATE_LOC_CENTROID;
interp_param_idx = lookup_interp_param_index(interp, location);
if (interp_param_idx == -1)
return;
else if (interp_param_idx)
interp_param = LLVMGetParam(ctx->main_fn, interp_param_idx);
else
interp_param = NULL;
- attr_number = LLVMConstInt(ctx->i32, input_index, 0);
-
if (inst->Instruction.Opcode == TGSI_OPCODE_INTERP_OFFSET ||
inst->Instruction.Opcode == TGSI_OPCODE_INTERP_SAMPLE) {
LLVMValueRef ij_out[2];
LLVMValueRef ddxy_out = si_llvm_emit_ddxy_interp(bld_base, interp_param);
/*
* take the I then J parameters, and the DDX/Y for it, and
* calculate the IJ inputs for the interpolator.
* temp1 = ddx * offset/sample.x + I;
* interp_param.I = ddy * offset/sample.y + temp1;
@@ -3584,42 +3605,59 @@ static void build_interp_intrinsic(const struct lp_build_tgsi_action *action,
temp1 = LLVMBuildFAdd(gallivm->builder, temp1, interp_el, "");
temp2 = LLVMBuildFMul(gallivm->builder, ddy_el, emit_data->args[1], "");
ij_out[i] = LLVMBuildFAdd(gallivm->builder, temp2, temp1, "");
}
interp_param = lp_build_gather_values(gallivm, ij_out, 2);
}
+ if (interp_param) {
+ interp_param = LLVMBuildBitCast(gallivm->builder,
+ interp_param, LLVMVectorType(ctx->f32, 2), "");
+ }
+
for (chan = 0; chan < 4; chan++) {
LLVMValueRef llvm_chan;
+ LLVMValueRef gather = LLVMGetUndef(LLVMVectorType(ctx->f32, input_array_size));
unsigned schan;
schan = tgsi_util_get_full_src_register_swizzle(&inst->Src[0], chan);
llvm_chan = LLVMConstInt(ctx->i32, schan, 0);
- if (interp_param) {
- interp_param = LLVMBuildBitCast(gallivm->builder,
- interp_param, LLVMVectorType(ctx->f32, 2), "");
- LLVMValueRef i = LLVMBuildExtractElement(
- gallivm->builder, interp_param, ctx->i32_0, "");
- LLVMValueRef j = LLVMBuildExtractElement(
- gallivm->builder, interp_param, ctx->i32_1, "");
- emit_data->output[chan] = ac_build_fs_interp(&ctx->ac,
- llvm_chan, attr_number, params,
- i, j);
- } else {
- emit_data->output[chan] = ac_build_fs_interp_mov(&ctx->ac,
- LLVMConstInt(ctx->i32, 2, 0), /* P0 */
- llvm_chan, attr_number, params);
+ for (unsigned i = 0; i < input_array_size; ++i) {
+ LLVMValueRef attr_number = LLVMConstInt(ctx->i32, input_base + i, false);
+ LLVMValueRef v;
+
+ if (interp_param) {
+ interp_param = LLVMBuildBitCast(gallivm->builder,
+ interp_param, LLVMVectorType(ctx->f32, 2), "");
+ LLVMValueRef i = LLVMBuildExtractElement(
+ gallivm->builder, interp_param, ctx->i32_0, "");
+ LLVMValueRef j = LLVMBuildExtractElement(
+ gallivm->builder, interp_param, ctx->i32_1, "");
+ v = ac_build_fs_interp(&ctx->ac,
+ llvm_chan, attr_number, params,
+ i, j);
+ } else {
+ v = ac_build_fs_interp_mov(&ctx->ac,
+ LLVMConstInt(ctx->i32, 2, 0), /* P0 */
+ llvm_chan, attr_number, params);
+ }
+
+ gather = LLVMBuildInsertElement(gallivm->builder,
+ gather, v, LLVMConstInt(ctx->i32, i, false), "");
}
+
+ emit_data->output[chan] = LLVMBuildExtractElement(
+ gallivm->builder, gather, array_idx, "");
}
}
static LLVMValueRef si_emit_ballot(struct si_shader_context *ctx,
LLVMValueRef value)
{
struct gallivm_state *gallivm = &ctx->gallivm;
LLVMValueRef args[3] = {
value,
ctx->i32_0,
--
2.9.3
_______________________________________________
mesa-dev mailing list
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Nicolai Hähnle
2017-06-16 20:37:14 UTC
Reply
Permalink
Raw Message
From: Nicolai Hähnle <***@amd.com>

Since interface blocks are simply considered groupings of input variables,
this is allowed.

var->data.must_be_shader_input is now determined on-the-fly after lowering
interface blocks, since we don't want to disable varying packing for an
entire block just because one input in it is used in interpolateAt*.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101378
---
src/compiler/glsl/ast_function.cpp | 7 +++++--
src/compiler/glsl/lower_named_interface_blocks.cpp | 18 ++++++++++++++++++
2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/src/compiler/glsl/ast_function.cpp b/src/compiler/glsl/ast_function.cpp
index 2d156ae..11897f7 100644
--- a/src/compiler/glsl/ast_function.cpp
+++ b/src/compiler/glsl/ast_function.cpp
@@ -221,29 +221,32 @@ verify_parameter_modes(_mesa_glsl_parse_state *state,
formal->name);
return false;
}
val = ((ir_swizzle *)val)->val;
}

while (val->ir_type == ir_type_dereference_array) {
val = ((ir_dereference_array *)val)->array;
}

+ if (const ir_dereference_record *deref = val->as_dereference_record()) {
+ if (deref->record->type->is_interface())
+ val = deref->record;
+ }
+
if (!val->as_dereference_variable() ||
val->variable_referenced()->data.mode != ir_var_shader_in) {
_mesa_glsl_error(&loc, state,
"parameter `%s` must be a shader input",
formal->name);
return false;
}
-
- val->variable_referenced()->data.must_be_shader_input = 1;
}

/* Verify that 'out' and 'inout' actual parameters are lvalues. */
if (formal->data.mode == ir_var_function_out
|| formal->data.mode == ir_var_function_inout) {
const char *mode = NULL;
switch (formal->data.mode) {
case ir_var_function_out: mode = "out"; break;
case ir_var_function_inout: mode = "inout"; break;
default: assert(false); break;
diff --git a/src/compiler/glsl/lower_named_interface_blocks.cpp b/src/compiler/glsl/lower_named_interface_blocks.cpp
index a00e60d..52e0571 100644
--- a/src/compiler/glsl/lower_named_interface_blocks.cpp
+++ b/src/compiler/glsl/lower_named_interface_blocks.cpp
@@ -108,20 +108,21 @@ public:

flatten_named_interface_blocks_declarations(void *mem_ctx)
: mem_ctx(mem_ctx),
interface_namespace(NULL)
{
}

void run(exec_list *instructions);

virtual ir_visitor_status visit_leave(ir_assignment *);
+ virtual ir_visitor_status visit_leave(ir_expression *);
virtual void handle_rvalue(ir_rvalue **rvalue);
};

} /* anonymous namespace */

void
flatten_named_interface_blocks_declarations::run(exec_list *instructions)
{
interface_namespace = _mesa_hash_table_create(NULL, _mesa_key_hash_string,
_mesa_key_string_equal);
@@ -231,20 +232,37 @@ flatten_named_interface_blocks_declarations::visit_leave(ir_assignment *ir)
}

ir_variable *lhs_var = lhs_rec_tmp->variable_referenced();
if (lhs_var) {
lhs_var->data.assigned = 1;
}
}
return rvalue_visit(ir);
}

+ir_visitor_status
+flatten_named_interface_blocks_declarations::visit_leave(ir_expression *ir)
+{
+ ir_visitor_status status = rvalue_visit(ir);
+
+ if (ir->operation == ir_unop_interpolate_at_centroid ||
+ ir->operation == ir_binop_interpolate_at_offset ||
+ ir->operation == ir_binop_interpolate_at_sample) {
+ const ir_rvalue *val = ir->operands[0];
+
+ /* This disables varying packing for this input. */
+ val->variable_referenced()->data.must_be_shader_input = 1;
+ }
+
+ return status;
+}
+
void
flatten_named_interface_blocks_declarations::handle_rvalue(ir_rvalue **rvalue)
{
if (*rvalue == NULL)
return;

ir_dereference_record *ir = (*rvalue)->as_dereference_record();
if (ir == NULL)
return;
--
2.9.3
Ian Romanick
2017-06-18 19:18:32 UTC
Reply
Permalink
Raw Message
Post by Nicolai Hähnle
Since interface blocks are simply considered groupings of input variables,
this is allowed.
var->data.must_be_shader_input is now determined on-the-fly after lowering
interface blocks, since we don't want to disable varying packing for an
entire block just because one input in it is used in interpolateAt*.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101378
I looked at the bug, but I haven't looked at the piglit list... is there
a test for this? We should add one.

I /think/ this should also be tagged for stable.
Post by Nicolai Hähnle
---
src/compiler/glsl/ast_function.cpp | 7 +++++--
src/compiler/glsl/lower_named_interface_blocks.cpp | 18 ++++++++++++++++++
2 files changed, 23 insertions(+), 2 deletions(-)
diff --git a/src/compiler/glsl/ast_function.cpp b/src/compiler/glsl/ast_function.cpp
index 2d156ae..11897f7 100644
--- a/src/compiler/glsl/ast_function.cpp
+++ b/src/compiler/glsl/ast_function.cpp
@@ -221,29 +221,32 @@ verify_parameter_modes(_mesa_glsl_parse_state *state,
formal->name);
return false;
}
val = ((ir_swizzle *)val)->val;
}
while (val->ir_type == ir_type_dereference_array) {
val = ((ir_dereference_array *)val)->array;
}
+ if (const ir_dereference_record *deref = val->as_dereference_record()) {
+ if (deref->record->type->is_interface())
+ val = deref->record;
+ }
+
if (!val->as_dereference_variable() ||
val->variable_referenced()->data.mode != ir_var_shader_in) {
_mesa_glsl_error(&loc, state,
"parameter `%s` must be a shader input",
formal->name);
return false;
}
-
- val->variable_referenced()->data.must_be_shader_input = 1;
I think I'd like to leave this here for non-interface blocks. I could
envision someone changing the linker to not call
lower_named_interface_blocks for shaders that can't have interface
blocks... then things would break in hard to find ways.
Post by Nicolai Hähnle
}
/* Verify that 'out' and 'inout' actual parameters are lvalues. */
if (formal->data.mode == ir_var_function_out
|| formal->data.mode == ir_var_function_inout) {
const char *mode = NULL;
switch (formal->data.mode) {
case ir_var_function_out: mode = "out"; break;
case ir_var_function_inout: mode = "inout"; break;
default: assert(false); break;
diff --git a/src/compiler/glsl/lower_named_interface_blocks.cpp b/src/compiler/glsl/lower_named_interface_blocks.cpp
index a00e60d..52e0571 100644
--- a/src/compiler/glsl/lower_named_interface_blocks.cpp
+++ b/src/compiler/glsl/lower_named_interface_blocks.cpp
flatten_named_interface_blocks_declarations(void *mem_ctx)
: mem_ctx(mem_ctx),
interface_namespace(NULL)
{
}
void run(exec_list *instructions);
virtual ir_visitor_status visit_leave(ir_assignment *);
+ virtual ir_visitor_status visit_leave(ir_expression *);
virtual void handle_rvalue(ir_rvalue **rvalue);
};
} /* anonymous namespace */
void
flatten_named_interface_blocks_declarations::run(exec_list *instructions)
{
interface_namespace = _mesa_hash_table_create(NULL, _mesa_key_hash_string,
_mesa_key_string_equal);
@@ -231,20 +232,37 @@ flatten_named_interface_blocks_declarations::visit_leave(ir_assignment *ir)
}
ir_variable *lhs_var = lhs_rec_tmp->variable_referenced();
if (lhs_var) {
lhs_var->data.assigned = 1;
}
}
return rvalue_visit(ir);
}
+ir_visitor_status
+flatten_named_interface_blocks_declarations::visit_leave(ir_expression *ir)
+{
+ ir_visitor_status status = rvalue_visit(ir);
+
+ if (ir->operation == ir_unop_interpolate_at_centroid ||
+ ir->operation == ir_binop_interpolate_at_offset ||
+ ir->operation == ir_binop_interpolate_at_sample) {
+ const ir_rvalue *val = ir->operands[0];
+
+ /* This disables varying packing for this input. */
+ val->variable_referenced()->data.must_be_shader_input = 1;
+ }
+
+ return status;
+}
+
void
flatten_named_interface_blocks_declarations::handle_rvalue(ir_rvalue **rvalue)
{
if (*rvalue == NULL)
return;
ir_dereference_record *ir = (*rvalue)->as_dereference_record();
if (ir == NULL)
return;
Nicolai Hähnle
2017-06-19 11:45:18 UTC
Reply
Permalink
Raw Message
Post by Ian Romanick
Post by Nicolai Hähnle
Since interface blocks are simply considered groupings of input variables,
this is allowed.
var->data.must_be_shader_input is now determined on-the-fly after lowering
interface blocks, since we don't want to disable varying packing for an
entire block just because one input in it is used in interpolateAt*.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101378
I looked at the bug, but I haven't looked at the piglit list... is there
a test for this? We should add one.
I sent a series that adds more tests.
Post by Ian Romanick
I /think/ this should also be tagged for stable.
Sure.
Post by Ian Romanick
Post by Nicolai Hähnle
---
src/compiler/glsl/ast_function.cpp | 7 +++++--
src/compiler/glsl/lower_named_interface_blocks.cpp | 18 ++++++++++++++++++
2 files changed, 23 insertions(+), 2 deletions(-)
diff --git a/src/compiler/glsl/ast_function.cpp b/src/compiler/glsl/ast_function.cpp
index 2d156ae..11897f7 100644
--- a/src/compiler/glsl/ast_function.cpp
+++ b/src/compiler/glsl/ast_function.cpp
@@ -221,29 +221,32 @@ verify_parameter_modes(_mesa_glsl_parse_state *state,
formal->name);
return false;
}
val = ((ir_swizzle *)val)->val;
}
while (val->ir_type == ir_type_dereference_array) {
val = ((ir_dereference_array *)val)->array;
}
+ if (const ir_dereference_record *deref = val->as_dereference_record()) {
+ if (deref->record->type->is_interface())
+ val = deref->record;
+ }
+
if (!val->as_dereference_variable() ||
val->variable_referenced()->data.mode != ir_var_shader_in) {
_mesa_glsl_error(&loc, state,
"parameter `%s` must be a shader input",
formal->name);
return false;
}
-
- val->variable_referenced()->data.must_be_shader_input = 1;
I think I'd like to leave this here for non-interface blocks. I could
envision someone changing the linker to not call
lower_named_interface_blocks for shaders that can't have interface
blocks... then things would break in hard to find ways.
Makes sense, changed locally.

Cheers,
Nicolai
Post by Ian Romanick
Post by Nicolai Hähnle
}
/* Verify that 'out' and 'inout' actual parameters are lvalues. */
if (formal->data.mode == ir_var_function_out
|| formal->data.mode == ir_var_function_inout) {
const char *mode = NULL;
switch (formal->data.mode) {
case ir_var_function_out: mode = "out"; break;
case ir_var_function_inout: mode = "inout"; break;
default: assert(false); break;
diff --git a/src/compiler/glsl/lower_named_interface_blocks.cpp b/src/compiler/glsl/lower_named_interface_blocks.cpp
index a00e60d..52e0571 100644
--- a/src/compiler/glsl/lower_named_interface_blocks.cpp
+++ b/src/compiler/glsl/lower_named_interface_blocks.cpp
flatten_named_interface_blocks_declarations(void *mem_ctx)
: mem_ctx(mem_ctx),
interface_namespace(NULL)
{
}
void run(exec_list *instructions);
virtual ir_visitor_status visit_leave(ir_assignment *);
+ virtual ir_visitor_status visit_leave(ir_expression *);
virtual void handle_rvalue(ir_rvalue **rvalue);
};
} /* anonymous namespace */
void
flatten_named_interface_blocks_declarations::run(exec_list *instructions)
{
interface_namespace = _mesa_hash_table_create(NULL, _mesa_key_hash_string,
_mesa_key_string_equal);
@@ -231,20 +232,37 @@ flatten_named_interface_blocks_declarations::visit_leave(ir_assignment *ir)
}
ir_variable *lhs_var = lhs_rec_tmp->variable_referenced();
if (lhs_var) {
lhs_var->data.assigned = 1;
}
}
return rvalue_visit(ir);
}
+ir_visitor_status
+flatten_named_interface_blocks_declarations::visit_leave(ir_expression *ir)
+{
+ ir_visitor_status status = rvalue_visit(ir);
+
+ if (ir->operation == ir_unop_interpolate_at_centroid ||
+ ir->operation == ir_binop_interpolate_at_offset ||
+ ir->operation == ir_binop_interpolate_at_sample) {
+ const ir_rvalue *val = ir->operands[0];
+
+ /* This disables varying packing for this input. */
+ val->variable_referenced()->data.must_be_shader_input = 1;
+ }
+
+ return status;
+}
+
void
flatten_named_interface_blocks_declarations::handle_rvalue(ir_rvalue **rvalue)
{
if (*rvalue == NULL)
return;
ir_dereference_record *ir = (*rvalue)->as_dereference_record();
if (ir == NULL)
return;
--
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.
Ian Romanick
2017-06-19 21:41:21 UTC
Reply
Permalink
Raw Message
Post by Nicolai Hähnle
Post by Ian Romanick
Post by Nicolai Hähnle
Since interface blocks are simply considered groupings of input variables,
this is allowed.
var->data.must_be_shader_input is now determined on-the-fly after lowering
interface blocks, since we don't want to disable varying packing for an
entire block just because one input in it is used in interpolateAt*.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101378
I looked at the bug, but I haven't looked at the piglit list... is there
a test for this? We should add one.
I sent a series that adds more tests.
Post by Ian Romanick
I /think/ this should also be tagged for stable.
Sure.
Post by Ian Romanick
Post by Nicolai Hähnle
---
src/compiler/glsl/ast_function.cpp | 7 +++++--
src/compiler/glsl/lower_named_interface_blocks.cpp | 18
++++++++++++++++++
2 files changed, 23 insertions(+), 2 deletions(-)
diff --git a/src/compiler/glsl/ast_function.cpp
b/src/compiler/glsl/ast_function.cpp
index 2d156ae..11897f7 100644
--- a/src/compiler/glsl/ast_function.cpp
+++ b/src/compiler/glsl/ast_function.cpp
@@ -221,29 +221,32 @@ verify_parameter_modes(_mesa_glsl_parse_state *state,
formal->name);
return false;
}
val = ((ir_swizzle *)val)->val;
}
while (val->ir_type == ir_type_dereference_array) {
val = ((ir_dereference_array *)val)->array;
}
+ if (const ir_dereference_record *deref =
val->as_dereference_record()) {
+ if (deref->record->type->is_interface())
+ val = deref->record;
+ }
+
if (!val->as_dereference_variable() ||
val->variable_referenced()->data.mode !=
ir_var_shader_in) {
_mesa_glsl_error(&loc, state,
"parameter `%s` must be a shader input",
formal->name);
return false;
}
-
- val->variable_referenced()->data.must_be_shader_input = 1;
I think I'd like to leave this here for non-interface blocks. I could
envision someone changing the linker to not call
lower_named_interface_blocks for shaders that can't have interface
blocks... then things would break in hard to find ways.
Makes sense, changed locally.
With that change, this patch is

Reviewed-by: Ian Romanick <***@intel.com>

I'll go take a look at the piglit tests too.

Loading...