Mesa (main): r300: don't check for unitialized reads when rewriting register
GitLab Mirror
gitlab-mirror at kemper.freedesktop.org
Sun Jun 5 21:47:43 UTC 2022
Module: Mesa
Branch: main
Commit: 0fcd423a6a54842cb37505c87b395f2983fd904d
URL: http://cgit.freedesktop.org/mesa/mesa/commit/?id=0fcd423a6a54842cb37505c87b395f2983fd904d
Author: Filip Gawin <filip.gawin at zoho.com>
Date: Sun Feb 13 00:28:36 2022 +0100
r300: don't check for unitialized reads when rewriting register
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 <pavel.ondracka at gmail.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/16657>
---
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 4ba0094cdea..8aa12cb4062 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 b609d9fae3c..5d368299231 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 462f920feaf..49b3e661eb0 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 6aec0940d80..c021f5cc1f5 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);
More information about the mesa-commit
mailing list