2017-06-26 13:40 GMT+02:00 Lucas Stach <l.st...@pengutronix.de>:
> The labels array may change its virtual address on a reallocation, so
> it is invalid to cache pointers into the array. Rather than using the
> pointer directly, remember the array index.
>
> Fixes miscompilation of shaders in glmark2 ideas, leading to GPU hangs.
>
> Signed-off-by: Lucas Stach <l.st...@pengutronix.de>
Reviewed-by: Christian Gmeiner <christian.gmei...@gmail.com>
> ---
> src/gallium/drivers/etnaviv/etnaviv_compiler.c | 57
> ++++++++++++++------------
> 1 file changed, 30 insertions(+), 27 deletions(-)
>
> diff --git a/src/gallium/drivers/etnaviv/etnaviv_compiler.c
> b/src/gallium/drivers/etnaviv/etnaviv_compiler.c
> index eafb511bb813..8883e39cf4b3 100644
> --- a/src/gallium/drivers/etnaviv/etnaviv_compiler.c
> +++ b/src/gallium/drivers/etnaviv/etnaviv_compiler.c
> @@ -119,10 +119,10 @@ enum etna_compile_frame_type {
> */
> struct etna_compile_frame {
> enum etna_compile_frame_type type;
> - struct etna_compile_label *lbl_else;
> - struct etna_compile_label *lbl_endif;
> - struct etna_compile_label *lbl_loop_bgn;
> - struct etna_compile_label *lbl_loop_end;
> + int lbl_else_idx;
> + int lbl_endif_idx;
> + int lbl_loop_bgn_idx;
> + int lbl_loop_end_idx;
> };
>
> struct etna_compile_file {
> @@ -178,7 +178,7 @@ struct etna_compile {
> /* Fields for handling nested conditionals */
> struct etna_compile_frame frame_stack[ETNA_MAX_DEPTH];
> int frame_sp;
> - struct etna_compile_label *lbl_usage[ETNA_MAX_INSTRUCTIONS];
> + int lbl_usage[ETNA_MAX_INSTRUCTIONS];
>
> unsigned labels_count, labels_sz;
> struct etna_compile_label *labels;
> @@ -990,7 +990,7 @@ etna_src_uniforms_conflict(struct etna_inst_src a, struct
> etna_inst_src b)
> }
>
> /* create a new label */
> -static struct etna_compile_label *
> +static unsigned int
> alloc_new_label(struct etna_compile *c)
> {
> struct etna_compile_label label = {
> @@ -999,7 +999,7 @@ alloc_new_label(struct etna_compile *c)
>
> array_insert(c->labels, label);
>
> - return &c->labels[c->labels_count - 1];
> + return c->labels_count - 1;
> }
>
> /* place label at current instruction pointer */
> @@ -1015,10 +1015,10 @@ label_place(struct etna_compile *c, struct
> etna_compile_label *label)
> * as the value becomes known.
> */
> static void
> -label_mark_use(struct etna_compile *c, struct etna_compile_label *label)
> +label_mark_use(struct etna_compile *c, int lbl_idx)
> {
> assert(c->inst_ptr < ETNA_MAX_INSTRUCTIONS);
> - c->lbl_usage[c->inst_ptr] = label;
> + c->lbl_usage[c->inst_ptr] = lbl_idx;
> }
>
> /* walk the frame stack and return first frame with matching type */
> @@ -1099,8 +1099,8 @@ trans_if(const struct instr_translater *t, struct
> etna_compile *c,
> /* push IF to stack */
> f->type = ETNA_COMPILE_FRAME_IF;
> /* create "else" label */
> - f->lbl_else = alloc_new_label(c);
> - f->lbl_endif = NULL;
> + f->lbl_else_idx = alloc_new_label(c);
> + f->lbl_endif_idx = -1;
>
> /* We need to avoid the emit_inst() below becoming two instructions */
> if (etna_src_uniforms_conflict(src[0], imm_0))
> @@ -1108,7 +1108,7 @@ trans_if(const struct instr_translater *t, struct
> etna_compile *c,
>
> /* mark position in instruction stream of label reference so that it can
> be
> * filled in in next pass */
> - label_mark_use(c, f->lbl_else);
> + label_mark_use(c, f->lbl_else_idx);
>
> /* create conditional branch to label if src0 EQ 0 */
> emit_inst(c, &(struct etna_inst){
> @@ -1129,8 +1129,8 @@ trans_else(const struct instr_translater *t, struct
> etna_compile *c,
> assert(f->type == ETNA_COMPILE_FRAME_IF);
>
> /* create "endif" label, and branch to endif label */
> - f->lbl_endif = alloc_new_label(c);
> - label_mark_use(c, f->lbl_endif);
> + f->lbl_endif_idx = alloc_new_label(c);
> + label_mark_use(c, f->lbl_endif_idx);
> emit_inst(c, &(struct etna_inst) {
> .opcode = INST_OPCODE_BRANCH,
> .cond = INST_CONDITION_TRUE,
> @@ -1138,7 +1138,7 @@ trans_else(const struct instr_translater *t, struct
> etna_compile *c,
> });
>
> /* mark "else" label at this position in instruction stream */
> - label_place(c, f->lbl_else);
> + label_place(c, &c->labels[f->lbl_else_idx]);
> }
>
> static void
> @@ -1151,10 +1151,10 @@ trans_endif(const struct instr_translater *t, struct
> etna_compile *c,
>
> /* assign "endif" or "else" (if no ELSE) label to current position in
> * instruction stream, pop IF */
> - if (f->lbl_endif != NULL)
> - label_place(c, f->lbl_endif);
> + if (f->lbl_endif_idx != -1)
> + label_place(c, &c->labels[f->lbl_endif_idx]);
> else
> - label_place(c, f->lbl_else);
> + label_place(c, &c->labels[f->lbl_else_idx]);
> }
>
> static void
> @@ -1166,10 +1166,10 @@ trans_loop_bgn(const struct instr_translater *t,
> struct etna_compile *c,
>
> /* push LOOP to stack */
> f->type = ETNA_COMPILE_FRAME_LOOP;
> - f->lbl_loop_bgn = alloc_new_label(c);
> - f->lbl_loop_end = alloc_new_label(c);
> + f->lbl_loop_bgn_idx = alloc_new_label(c);
> + f->lbl_loop_end_idx = alloc_new_label(c);
>
> - label_place(c, f->lbl_loop_bgn);
> + label_place(c, &c->labels[f->lbl_loop_bgn_idx]);
>
> c->num_loops++;
> }
> @@ -1185,7 +1185,7 @@ trans_loop_end(const struct instr_translater *t, struct
> etna_compile *c,
>
> /* mark position in instruction stream of label reference so that it can
> be
> * filled in in next pass */
> - label_mark_use(c, f->lbl_loop_bgn);
> + label_mark_use(c, f->lbl_loop_bgn_idx);
>
> /* create branch to loop_bgn label */
> emit_inst(c, &(struct etna_inst) {
> @@ -1195,7 +1195,7 @@ trans_loop_end(const struct instr_translater *t, struct
> etna_compile *c,
> /* imm is filled in later */
> });
>
> - label_place(c, f->lbl_loop_end);
> + label_place(c, &c->labels[f->lbl_loop_end_idx]);
> }
>
> static void
> @@ -1207,7 +1207,7 @@ trans_brk(const struct instr_translater *t, struct
> etna_compile *c,
>
> /* mark position in instruction stream of label reference so that it can
> be
> * filled in in next pass */
> - label_mark_use(c, f->lbl_loop_end);
> + label_mark_use(c, f->lbl_loop_end_idx);
>
> /* create branch to loop_end label */
> emit_inst(c, &(struct etna_inst) {
> @@ -1227,7 +1227,7 @@ trans_cont(const struct instr_translater *t, struct
> etna_compile *c,
>
> /* mark position in instruction stream of label reference so that it can
> be
> * filled in in next pass */
> - label_mark_use(c, f->lbl_loop_bgn);
> + label_mark_use(c, f->lbl_loop_bgn_idx);
>
> /* create branch to loop_end label */
> emit_inst(c, &(struct etna_inst) {
> @@ -1998,8 +1998,9 @@ static void
> etna_compile_fill_in_labels(struct etna_compile *c)
> {
> for (int idx = 0; idx < c->inst_ptr; ++idx) {
> - if (c->lbl_usage[idx])
> - etna_assemble_set_imm(&c->code[idx * 4],
> c->lbl_usage[idx]->inst_idx);
> + if (c->lbl_usage[idx] != -1)
> + etna_assemble_set_imm(&c->code[idx * 4],
> + c->labels[c->lbl_usage[idx]].inst_idx);
> }
> }
>
> @@ -2301,6 +2302,8 @@ etna_compile_shader(struct etna_shader_variant *v)
> if (!c)
> return false;
>
> + memset(&c->lbl_usage, -1, ARRAY_SIZE(c->lbl_usage));
> +
> const struct tgsi_token *tokens = v->shader->tokens;
>
> c->specs = specs;
> --
> 2.11.0
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Passes all of my internal tgsi compiler tests (which I really should
push to github). Will
add a test case for the problematic shader.
greets
--
Christian Gmeiner, MSc
https://www.youtube.com/user/AloryOFFICIAL
https://soundcloud.com/christian-gmeiner
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
