[Mesa-dev] [PATCH v3] i965/vec4: Change types as needed to propagate source modifiers using from instruction
Alejandro Piñeiro
apinheiro at igalia.com
Fri Sep 18 08:12:11 PDT 2015
SEL and MOV instructions, as long as they don't have source modifiers, are
just copying bits around. So those kind of instruction could be propagated
even if there are type mismatches. This is needed because NIR generates
integer SEL and MOV instructions whenever it doesn't know what else to
generate.
This commit adds support for copy propagation using previous instruction
as reference.
v2: using 'arg' index to get the from inst was wrong, as pointed
by Jason Ekstrand
v3: rebased against last change on the previous patch of theh series
---
v3 is just a rebase against the first patch on the series. I also
realized that yesterday I did a mess when I sent the v2 of this
patch (answering to the wrong email, not including v3 on the subject),
so I will copy the text I had on that email here. Sorry for the noise.
>From the review:
>> if (has_source_modifiers &&
>> value.type != inst->src[arg].type &&
>> - !can_change_source_types(inst))
>> + !can_change_source_types(inst) &&
>> + !can_change_source_types(entry->inst[arg]))
> This isn't right. The entry->inst array is indexed on vec4 component
> but arg is the argument of the instruction. I think what you want to
> do is add something to the loop above to loop over 0...3 and check
> them all.
Yes you are right. At that moment I misunderstood 'arg' as the channel
being used at that moment, and as piglit didn't correct me with a
regression I never realized that I was wrong. This v2 computes
the instruction we are looking for corrrectly, with the a loop
as suggested.
> Also, how this is different from has_source_modifiers?
It is different because at that moment has_source_modifiers has
been computed using the src_reg value that had their modifiers
changed to the ones at the current string:
if (inst->src[arg].abs) {
value.negate = false;
value.abs = true;
}
if (inst->src[arg].negate)
value.negate = !value.negate;
bool has_source_modifiers = value.negate || value.abs;
While can_change_source_types with the from instruction is
using the original ones, and checking that is a mov, so "safe"
to going on with the propagation.
That means that there is an alternative to track the instructions
at copy_entry. I could maintain a copy of value without being
modified, or compute has_source_modifiers (ie: original_has_source_modifiers).
But I would still need to check if the opcode is a MOV on the from
instruction, so I would need to track opcode on the copy_entry struct.
Tracking inst also have the advantage that I can reuse the
function can_change_source_types.
> Obviously, it is (the shader-db numbers say so) but I'm not seeing it.
> Could you please provide an example.
Ok. On a shader like this:
uniform vec4 inData;
void main(void)
{
gl_Position = gl_Vertex - inData;
}
VS-0001-00-start is like this:
0: mov vgrf0.0:F, attr0.xyzw:F
1: mov vgrf1.0:UD, u0.xyzw:UD
2: add vgrf2.0:F, vgrf0.xyzw:F, -vgrf1.xyzw:F
3: mov m2:D, 0D
4: mov m3:F, vgrf2.xyzw:F
5: vs_urb_write (null):UD
When we are at instruction #2, has_source_modifiers is true
even for the value u0 because as I mentioned, it is copying the
modifiers at the same instruction (the modifiers of
vgrf1 at #2). But in any case, it is "safe" to do the copy
propagation from instruction #1 to instruction #2.
.../drivers/dri/i965/brw_vec4_copy_propagation.cpp | 47 ++++++++++++++++------
1 file changed, 35 insertions(+), 12 deletions(-)
diff --git a/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp b/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp
index 1522eea..dabc25f 100644
--- a/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp
+++ b/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp
@@ -39,6 +39,7 @@ namespace brw {
struct copy_entry {
src_reg *value[4];
+ vec4_instruction *inst[4];
int saturatemask;
};
@@ -251,7 +252,8 @@ try_constant_propagate(const struct brw_device_info *devinfo,
static bool
can_change_source_types(vec4_instruction *inst)
{
- return inst->dst.type == inst->src[0].type &&
+ return inst != NULL &&
+ inst->dst.type == inst->src[0].type &&
!inst->src[0].abs && !inst->src[0].negate && !inst->saturate &&
(inst->opcode == BRW_OPCODE_MOV ||
(inst->opcode == BRW_OPCODE_SEL &&
@@ -271,6 +273,8 @@ try_copy_propagate(const struct brw_device_info *devinfo,
* vectors better.
*/
src_reg value = *entry->value[0];
+ vec4_instruction *from_inst = entry->inst[0];
+
for (int i = 1; i < 4; i++) {
/* This is equals() except we don't care about the swizzle. */
if (value.file != entry->value[i]->file ||
@@ -283,6 +287,13 @@ try_copy_propagate(const struct brw_device_info *devinfo,
}
}
+ for (int i = 1; i < 4; i++) {
+ if (from_inst != entry->inst[i]) {
+ from_inst = NULL;
+ break;
+ }
+ }
+
/* Compute the swizzle of the original register by swizzling the
* component loaded from each value according to the swizzle of
* operand we're going to change.
@@ -322,7 +333,8 @@ try_copy_propagate(const struct brw_device_info *devinfo,
if (has_source_modifiers &&
value.type != inst->src[arg].type &&
- !can_change_source_types(inst))
+ !can_change_source_types(inst) &&
+ !can_change_source_types(from_inst))
return false;
if (has_source_modifiers &&
@@ -340,7 +352,8 @@ try_copy_propagate(const struct brw_device_info *devinfo,
* instead. See also resolve_ud_negate().
*/
if (value.negate &&
- value.type == BRW_REGISTER_TYPE_UD)
+ value.type == BRW_REGISTER_TYPE_UD &&
+ !can_change_source_types(from_inst))
return false;
/* Don't report progress if this is a noop. */
@@ -378,17 +391,25 @@ try_copy_propagate(const struct brw_device_info *devinfo,
if (has_source_modifiers &&
value.type != inst->src[arg].type) {
- /* We are propagating source modifiers from a MOV with a different
- * type. If we got here, then we can just change the source and
- * destination types of the instruction and keep going.
+ /* We are propagating source modifiers from a safe instruction with a
+ * different type. If we got here, then we can just change the source
+ * and destination types of the current instruction or the instruction
+ * from we are propagating.
*/
- assert(can_change_source_types(inst));
- for (int i = 0; i < 3; i++) {
- inst->src[i].type = value.type;
+ assert(can_change_source_types(inst) ||
+ can_change_source_types(from_inst));
+
+ if (can_change_source_types(inst)) {
+ for (int i = 0; i < 3; i++) {
+ inst->src[i].type = value.type;
+ }
+ inst->dst.type = value.type;
+ } else {
+ value.type = inst->src[arg].type;
}
- inst->dst.type = value.type;
- } else
+ } else {
value.type = inst->src[arg].type;
+ }
inst->src[arg] = value;
return true;
}
@@ -441,7 +462,7 @@ vec4_visitor::opt_copy_propagation(bool do_constant_prop)
for (c = 0; c < 4; c++) {
int channel = BRW_GET_SWZ(inst->src[i].swizzle, c);
entry.value[c] = entries[reg].value[channel];
-
+ entry.inst[c] = entries[reg].inst[channel];
/* If there's no available copy for this channel, bail.
* We could be more aggressive here -- some channels might
* not get used based on the destination writemask.
@@ -486,6 +507,7 @@ vec4_visitor::opt_copy_propagation(bool do_constant_prop)
entries[reg].value[i] = direct_copy ? &inst->src[0] : NULL;
entries[reg].saturatemask |=
inst->saturate && direct_copy ? 1 << i : 0;
+ entries[reg].inst[i] = direct_copy ? inst : NULL;
}
}
@@ -500,6 +522,7 @@ vec4_visitor::opt_copy_propagation(bool do_constant_prop)
if (is_channel_updated(inst, entries[i].value, j)) {
entries[i].value[j] = NULL;
entries[i].saturatemask &= ~(1 << j);
+ entries[i].inst[j] = NULL;
}
}
}
--
2.1.4
More information about the mesa-dev
mailing list