From 0fcd423a6a54842cb37505c87b395f2983fd904d Mon Sep 17 00:00:00 2001 From: Filip Gawin Date: Sun, 13 Feb 2022 00:28:36 +0100 Subject: [PATCH] r300: don't check for unitialized reads when rewriting register MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit This fixes the "Rewrite of inst X failed Can't allocate source for Inst X src_type=X new_index=X new_mask=X" errors. The compiler is quite strict when rewriting registers during the pair allocation and checks that all of the reads of it are initialized. However the spec doesn't enfore that, and specifically with control flow depending on user input we can't really know... In the following example temp[4].x is written only in one branch, that might or might not be taken, but this is enough to keep the compiler happy: IF aluresult.x___; MAD temp[4].x, src0.1__, src0.111, src0.000 ENDIF; src0.xyz = temp[4], src0.w = temp[4] MAD color[0].xyz, src0.xyz, src0.111, src0.000 MAD color[0].w, src0.w, src0.1, src0.0 After switch to ntt, more IFs are converted to CMP, and the color write looks like this. Please note that the CMP here is not TGSI opcode but rather our US_OP_RGB_CMP: src2 >= 0 ? src0 : src1 src0.xyz = temp[4], src0.w = temp[4], src1.xyz = temp[3], src1.w = temp[12], src2.xyz = temp[2] CMP color[0].xyz, src0.xyz, src1.xyz, -src2.xxx CMP color[0].w, src0.w, src1.w, -src2.x At this point temp[4].x is undefined. Now when compiler tries to allocate register for temp[4] at some previous instruction, it will find out that it is used as a source in the final CMP and bail out. Instead of increasing the complexitty even more trying to account for this, just get rid of the check completelly. Fixes: dEQP-GLES2.functional.shaders.indexing.vector_subscript.vec2_dynamic_subscript_write_component_read_fragment,Fail dEQP-GLES2.functional.shaders.indexing.vector_subscript.vec2_dynamic_subscript_write_direct_read_fragment,Fail dEQP-GLES2.functional.shaders.indexing.vector_subscript.vec2_dynamic_subscript_write_dynamic_subscript_read_fragment,Fail dEQP-GLES2.functional.shaders.indexing.vector_subscript.vec2_dynamic_subscript_write_static_loop_subscript_read_fragment,Fail dEQP-GLES2.functional.shaders.indexing.vector_subscript.vec2_dynamic_subscript_write_static_subscript_read_fragment,Fail dEQP-GLES2.functional.shaders.indexing.vector_subscript.vec3_dynamic_subscript_write_component_read_fragment,Fail dEQP-GLES2.functional.shaders.indexing.vector_subscript.vec3_dynamic_subscript_write_direct_read_fragment,Fail dEQP-GLES2.functional.shaders.indexing.vector_subscript.vec3_dynamic_subscript_write_dynamic_subscript_read_fragment,Fail dEQP-GLES2.functional.shaders.indexing.vector_subscript.vec3_dynamic_subscript_write_static_loop_subscript_read_fragment,Fail dEQP-GLES2.functional.shaders.indexing.vector_subscript.vec3_dynamic_subscript_write_static_subscript_read_fragment,Fail dEQP-GLES2.functional.shaders.indexing.vector_subscript.vec4_dynamic_subscript_write_component_read_fragment,Fail dEQP-GLES2.functional.shaders.indexing.vector_subscript.vec4_dynamic_subscript_write_direct_read_fragment,Fail dEQP-GLES2.functional.shaders.indexing.vector_subscript.vec4_dynamic_subscript_write_dynamic_subscript_read_fragment,Fail dEQP-GLES2.functional.shaders.indexing.vector_subscript.vec4_dynamic_subscript_write_static_loop_subscript_read_fragment,Fail dEQP-GLES2.functional.shaders.indexing.vector_subscript.vec4_dynamic_subscript_write_static_subscript_read_fragment,Fail Reviewed-by: Pavel Ondračka Part-of: --- src/gallium/drivers/r300/ci/r300-rv515-fails.txt | 17 ------ .../drivers/r300/compiler/radeon_compiler_util.c | 42 ++------------ .../drivers/r300/compiler/radeon_compiler_util.h | 6 +- .../drivers/r300/compiler/radeon_variable.c | 64 ++++++++-------------- 4 files changed, 28 insertions(+), 101 deletions(-) diff --git a/src/gallium/drivers/r300/ci/r300-rv515-fails.txt b/src/gallium/drivers/r300/ci/r300-rv515-fails.txt index 4ba0094..8aa12cb 100644 --- a/src/gallium/drivers/r300/ci/r300-rv515-fails.txt +++ b/src/gallium/drivers/r300/ci/r300-rv515-fails.txt @@ -50,23 +50,6 @@ dEQP-GLES2.functional.shaders.indexing.tmp_array.vec2_const_write_dynamic_read_f dEQP-GLES2.functional.shaders.indexing.tmp_array.vec3_const_write_dynamic_read_fragment,Fail dEQP-GLES2.functional.shaders.indexing.tmp_array.vec4_const_write_dynamic_read_fragment,Fail -# "Rewrite of inst 0 failed Can't allocate source for Inst 4 src_type=1 new_index=1 new_mask=1" -dEQP-GLES2.functional.shaders.indexing.vector_subscript.vec2_dynamic_subscript_write_component_read_fragment,Fail -dEQP-GLES2.functional.shaders.indexing.vector_subscript.vec2_dynamic_subscript_write_direct_read_fragment,Fail -dEQP-GLES2.functional.shaders.indexing.vector_subscript.vec2_dynamic_subscript_write_dynamic_subscript_read_fragment,Fail -dEQP-GLES2.functional.shaders.indexing.vector_subscript.vec2_dynamic_subscript_write_static_loop_subscript_read_fragment,Fail -dEQP-GLES2.functional.shaders.indexing.vector_subscript.vec2_dynamic_subscript_write_static_subscript_read_fragment,Fail -dEQP-GLES2.functional.shaders.indexing.vector_subscript.vec3_dynamic_subscript_write_component_read_fragment,Fail -dEQP-GLES2.functional.shaders.indexing.vector_subscript.vec3_dynamic_subscript_write_direct_read_fragment,Fail -dEQP-GLES2.functional.shaders.indexing.vector_subscript.vec3_dynamic_subscript_write_dynamic_subscript_read_fragment,Fail -dEQP-GLES2.functional.shaders.indexing.vector_subscript.vec3_dynamic_subscript_write_static_loop_subscript_read_fragment,Fail -dEQP-GLES2.functional.shaders.indexing.vector_subscript.vec3_dynamic_subscript_write_static_subscript_read_fragment,Fail -dEQP-GLES2.functional.shaders.indexing.vector_subscript.vec4_dynamic_subscript_write_component_read_fragment,Fail -dEQP-GLES2.functional.shaders.indexing.vector_subscript.vec4_dynamic_subscript_write_direct_read_fragment,Fail -dEQP-GLES2.functional.shaders.indexing.vector_subscript.vec4_dynamic_subscript_write_dynamic_subscript_read_fragment,Fail -dEQP-GLES2.functional.shaders.indexing.vector_subscript.vec4_dynamic_subscript_write_static_loop_subscript_read_fragment,Fail -dEQP-GLES2.functional.shaders.indexing.vector_subscript.vec4_dynamic_subscript_write_static_subscript_read_fragment,Fail - dEQP-GLES2.functional.shaders.random.texture.fragment.141,Fail dEQP-GLES2.functional.shaders.return.return_in_dynamic_loop_dynamic_fragment,Fail diff --git a/src/gallium/drivers/r300/compiler/radeon_compiler_util.c b/src/gallium/drivers/r300/compiler/radeon_compiler_util.c index b609d9f..5d36829 100644 --- a/src/gallium/drivers/r300/compiler/radeon_compiler_util.c +++ b/src/gallium/drivers/r300/compiler/radeon_compiler_util.c @@ -550,50 +550,18 @@ int rc_get_max_index( } } -static unsigned int get_source_readmask( - struct rc_pair_sub_instruction * sub, - unsigned int source, - unsigned int src_type) -{ - unsigned int i; - unsigned int readmask = 0; - const struct rc_opcode_info * info = rc_get_opcode_info(sub->Opcode); - - for (i = 0; i < info->NumSrcRegs; i++) { - if (sub->Arg[i].Source != source - || src_type != rc_source_type_swz(sub->Arg[i].Swizzle)) { - continue; - } - readmask |= rc_swizzle_to_writemask(sub->Arg[i].Swizzle); - } - return readmask; -} - /** - * This function attempts to remove a source from a pair instructions. + * This function removes a source from a pair instructions. * @param inst * @param src_type RC_SOURCE_RGB, RC_SOURCE_ALPHA, or both bitwise or'd * @param source The index of the source to remove - * @param new_readmask A mask representing the components that are read by - * the source that is intended to replace the one you are removing. If you - * want to remove a source only and not replace it, this parameter should be - * zero. - * @return 1 if the source was successfully removed, 0 if it was not + */ -unsigned int rc_pair_remove_src( +void rc_pair_remove_src( struct rc_instruction * inst, unsigned int src_type, - unsigned int source, - unsigned int new_readmask) + unsigned int source) { - unsigned int readmask = 0; - - readmask |= get_source_readmask(&inst->U.P.RGB, source, src_type); - readmask |= get_source_readmask(&inst->U.P.Alpha, source, src_type); - - if ((new_readmask & readmask) != readmask) - return 0; - if (src_type & RC_SOURCE_RGB) { memset(&inst->U.P.RGB.Src[source], 0, sizeof(struct rc_pair_instruction_source)); @@ -603,8 +571,6 @@ unsigned int rc_pair_remove_src( memset(&inst->U.P.Alpha.Src[source], 0, sizeof(struct rc_pair_instruction_source)); } - - return 1; } /** diff --git a/src/gallium/drivers/r300/compiler/radeon_compiler_util.h b/src/gallium/drivers/r300/compiler/radeon_compiler_util.h index 462f920..49b3e66 100644 --- a/src/gallium/drivers/r300/compiler/radeon_compiler_util.h +++ b/src/gallium/drivers/r300/compiler/radeon_compiler_util.h @@ -98,11 +98,9 @@ int rc_get_max_index( struct radeon_compiler * c, rc_register_file file); -unsigned int rc_pair_remove_src( - struct rc_instruction * inst, +void rc_pair_remove_src(struct rc_instruction * inst, unsigned int src_type, - unsigned int source, - unsigned int new_readmask); + unsigned int source); rc_opcode rc_get_flow_control_inst(struct rc_instruction * inst); diff --git a/src/gallium/drivers/r300/compiler/radeon_variable.c b/src/gallium/drivers/r300/compiler/radeon_variable.c index 6aec094..c021f5c 100644 --- a/src/gallium/drivers/r300/compiler/radeon_variable.c +++ b/src/gallium/drivers/r300/compiler/radeon_variable.c @@ -89,48 +89,28 @@ void rc_variable_change_dst( src_index = rc_pair_get_src_index( pair_inst, reader->U.P.Src); } - /* Try to delete the old src, it is OK if this fails, - * because rc_pair_alloc_source might be able to - * find a source the ca be reused. - */ - if (rc_pair_remove_src(reader->Inst, src_type, - src_index, old_mask)) { - /* Reuse the source index of the source that - * was just deleted and set its register - * index. We can't use rc_pair_alloc_source - * for this because it might return a source - * index that is already being used. */ - if (src_type & RC_SOURCE_RGB) { - pair_inst->RGB.Src[src_index] - .Used = 1; - pair_inst->RGB.Src[src_index] - .Index = new_index; - pair_inst->RGB.Src[src_index] - .File = RC_FILE_TEMPORARY; - } - if (src_type & RC_SOURCE_ALPHA) { - pair_inst->Alpha.Src[src_index] - .Used = 1; - pair_inst->Alpha.Src[src_index] - .Index = new_index; - pair_inst->Alpha.Src[src_index] - .File = RC_FILE_TEMPORARY; - } - } else { - src_index = rc_pair_alloc_source( - &reader->Inst->U.P, - src_type & RC_SOURCE_RGB, - src_type & RC_SOURCE_ALPHA, - RC_FILE_TEMPORARY, - new_index); - if (src_index < 0) { - rc_error(var->C, "Rewrite of inst %u failed " - "Can't allocate source for " - "Inst %u src_type=%x " - "new_index=%u new_mask=%u\n", - var->Inst->IP, reader->Inst->IP, src_type, new_index, new_writemask); - continue; - } + rc_pair_remove_src(reader->Inst, src_type, + src_index); + /* Reuse the source index of the source that + * was just deleted and set its register + * index. We can't use rc_pair_alloc_source + * for this because it might return a source + * index that is already being used. */ + if (src_type & RC_SOURCE_RGB) { + pair_inst->RGB.Src[src_index] + .Used = 1; + pair_inst->RGB.Src[src_index] + .Index = new_index; + pair_inst->RGB.Src[src_index] + .File = RC_FILE_TEMPORARY; + } + if (src_type & RC_SOURCE_ALPHA) { + pair_inst->Alpha.Src[src_index] + .Used = 1; + pair_inst->Alpha.Src[src_index] + .Index = new_index; + pair_inst->Alpha.Src[src_index] + .File = RC_FILE_TEMPORARY; } reader->U.P.Arg->Swizzle = rc_rewrite_swizzle( reader->U.P.Arg->Swizzle, conversion_swizzle); -- 2.7.4