From 52eb47c8d4840aa4fa74e869beccf29e812d7d65 Mon Sep 17 00:00:00 2001 From: Ian Romanick Date: Sun, 6 Dec 2020 16:26:04 -0800 Subject: [PATCH] intel/compiler: Relax some conditions in try_copy_propagate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously can_do_source_mods was used to determine whether a value with a source modifier or a value from a scalar source (e.g., a uniform) could be copy propagated. The former is a superset of the latter, so this always produces correct results, but it is overly restrictive. For example, a BFI instruction can't have source modifiers, but it can have scalar sources. This was originally authored to prevent a small number of shader-db regressions in a commit that marked SHR has not being able to have source modifiers. That commit has since been dropped in favor of a different method. v2: Refactor register region restriction detection to a helper function. Suggested by Jason. No fossil-db changes on any Intel platform. All Gen7+ platforms had similar results. (Ice Lake shown) total instructions in shared programs: 20039111 -> 20038943 (<.01%) instructions in affected programs: 31736 -> 31568 (-0.53%) helped: 104 HURT: 0 helped stats (abs) min: 1 max: 9 x̄: 1.62 x̃: 1 helped stats (rel) min: 0.30% max: 0.88% x̄: 0.45% x̃: 0.42% 95% mean confidence interval for instructions value: -2.03 -1.20 95% mean confidence interval for instructions %-change: -0.47% -0.42% Instructions are helped. total cycles in shared programs: 980309750 -> 980308897 (<.01%) cycles in affected programs: 591078 -> 590225 (-0.14%) helped: 70 HURT: 26 helped stats (abs) min: 2 max: 622 x̄: 23.94 x̃: 4 helped stats (rel) min: <.01% max: 2.85% x̄: 0.33% x̃: 0.12% HURT stats (abs) min: 2 max: 520 x̄: 31.65 x̃: 6 HURT stats (rel) min: 0.02% max: 2.45% x̄: 0.34% x̃: 0.15% 95% mean confidence interval for cycles value: -26.41 8.64 95% mean confidence interval for cycles %-change: -0.27% -0.03% Inconclusive result (value mean confidence interval includes 0). No shader-db changes on earlier Intel platforms. Reviewed-by: Anuj Phogat anuj.phogat@gmail.com [v1] Reviewed-by: Jason Ekstrand Part-of: --- src/intel/compiler/brw_fs_copy_propagation.cpp | 12 +++++++++--- src/intel/compiler/brw_ir.h | 6 ++++++ src/intel/compiler/brw_shader.cpp | 13 +++++++++++++ src/intel/compiler/brw_vec4_copy_propagation.cpp | 11 +++++++++-- 4 files changed, 37 insertions(+), 5 deletions(-) diff --git a/src/intel/compiler/brw_fs_copy_propagation.cpp b/src/intel/compiler/brw_fs_copy_propagation.cpp index 6896987055f..343bac23889 100644 --- a/src/intel/compiler/brw_fs_copy_propagation.cpp +++ b/src/intel/compiler/brw_fs_copy_propagation.cpp @@ -510,11 +510,17 @@ fs_visitor::try_copy_propagate(fs_inst *inst, int arg, acp_entry *entry) bool has_source_modifiers = entry->src.abs || entry->src.negate; - if ((has_source_modifiers || entry->src.file == UNIFORM || - !entry->src.is_contiguous()) && - !inst->can_do_source_mods(devinfo)) + if (has_source_modifiers && !inst->can_do_source_mods(devinfo)) return false; + /* Reject cases that would violate register regioning restrictions. */ + if ((entry->src.file == UNIFORM || !entry->src.is_contiguous()) && + ((devinfo->gen == 6 && inst->is_math()) || + inst->is_send_from_grf() || + inst->uses_indirect_addressing())) { + return false; + } + if (has_source_modifiers && inst->opcode == SHADER_OPCODE_GEN4_SCRATCH_WRITE) return false; diff --git a/src/intel/compiler/brw_ir.h b/src/intel/compiler/brw_ir.h index 01f81feddb4..a98eab9f8fb 100644 --- a/src/intel/compiler/brw_ir.h +++ b/src/intel/compiler/brw_ir.h @@ -101,6 +101,12 @@ struct backend_instruction : public exec_node { bool reads_accumulator_implicitly() const; bool writes_accumulator_implicitly(const struct gen_device_info *devinfo) const; + /** + * Instructions that use indirect addressing have additional register + * regioning restrictions. + */ + bool uses_indirect_addressing() const; + void remove(bblock_t *block); void insert_after(bblock_t *block, backend_instruction *inst); void insert_before(bblock_t *block, backend_instruction *inst); diff --git a/src/intel/compiler/brw_shader.cpp b/src/intel/compiler/brw_shader.cpp index f52295fa310..cba0b589dde 100644 --- a/src/intel/compiler/brw_shader.cpp +++ b/src/intel/compiler/brw_shader.cpp @@ -931,6 +931,19 @@ backend_instruction::is_control_flow() const } } +bool +backend_instruction::uses_indirect_addressing() const +{ + switch (opcode) { + case SHADER_OPCODE_BROADCAST: + case SHADER_OPCODE_CLUSTER_BROADCAST: + case SHADER_OPCODE_MOV_INDIRECT: + return true; + default: + return false; + } +} + bool backend_instruction::can_do_source_mods() const { diff --git a/src/intel/compiler/brw_vec4_copy_propagation.cpp b/src/intel/compiler/brw_vec4_copy_propagation.cpp index 9e4637e9753..a0df115d4a3 100644 --- a/src/intel/compiler/brw_vec4_copy_propagation.cpp +++ b/src/intel/compiler/brw_vec4_copy_propagation.cpp @@ -349,10 +349,17 @@ try_copy_propagate(const struct gen_device_info *devinfo, /* gen6 math and gen7+ SENDs from GRFs ignore source modifiers on * instructions. */ - if ((has_source_modifiers || value.file == UNIFORM || - value.swizzle != BRW_SWIZZLE_XYZW) && !inst->can_do_source_mods(devinfo)) + if (has_source_modifiers && !inst->can_do_source_mods(devinfo)) return false; + /* Reject cases that would violate register regioning restrictions. */ + if ((value.file == UNIFORM || value.swizzle != BRW_SWIZZLE_XYZW) && + ((devinfo->gen == 6 && inst->is_math()) || + inst->is_send_from_grf() || + inst->uses_indirect_addressing())) { + return false; + } + if (has_source_modifiers && value.type != inst->src[arg].type && !inst->can_change_types())