Discussion:
[Mesa-dev] [PATCH] intel/fs: Properly copy default flag reg for 3src instrucitons
Jason Ekstrand
2018-05-23 16:13:09 UTC
Permalink
Prior to gen8, the flag reg and subreg numbers are in different
locations on 3src instructions than on smaller instructions. In order
for brw_set_default_flag_reg to work properly, we need to copy the value
out of the 2src location and write it into the 3src location as part of
brw_alu3.

Cc: mesa-***@lists.freedesktop.org
---
src/intel/compiler/brw_eu_emit.c | 12 ++++++++++++
1 file changed, 12 insertions(+)

diff --git a/src/intel/compiler/brw_eu_emit.c b/src/intel/compiler/brw_eu_emit.c
index 4f51d51..fc39d94 100644
--- a/src/intel/compiler/brw_eu_emit.c
+++ b/src/intel/compiler/brw_eu_emit.c
@@ -824,6 +824,18 @@ brw_alu3(struct brw_codegen *p, unsigned opcode, struct brw_reg dest,
dest.type == BRW_REGISTER_TYPE_DF ||
dest.type == BRW_REGISTER_TYPE_D ||
dest.type == BRW_REGISTER_TYPE_UD);
+
+ /* Flag registers are in a different spot on 3src instructions so we
+ * need to move the value if we want brw_set_default_flag_reg to work
+ * properly.
+ */
+ unsigned flag_reg_nr =
+ devinfo->gen >= 7 ? brw_inst_flag_reg_nr(devinfo, inst) : 0;
+ unsigned flag_subreg_nr = brw_inst_flag_subreg_nr(devinfo, inst);
+ if (devinfo->gen >= 7)
+ brw_inst_set_3src_a16_flag_reg_nr(devinfo, inst, flag_reg_nr);
+ brw_inst_set_3src_a16_flag_subreg_nr(devinfo, inst, flag_subreg_nr);
+
if (devinfo->gen == 6) {
brw_inst_set_3src_a16_dst_reg_file(devinfo, inst,
dest.file == BRW_MESSAGE_REGISTER_FILE);
--
2.5.0.400.gff86faf
Francisco Jerez
2018-05-23 19:13:59 UTC
Permalink
Post by Jason Ekstrand
Prior to gen8, the flag reg and subreg numbers are in different
locations on 3src instructions than on smaller instructions. In order
for brw_set_default_flag_reg to work properly, we need to copy the value
out of the 2src location and write it into the 3src location as part of
brw_alu3.
---
src/intel/compiler/brw_eu_emit.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/src/intel/compiler/brw_eu_emit.c b/src/intel/compiler/brw_eu_emit.c
index 4f51d51..fc39d94 100644
--- a/src/intel/compiler/brw_eu_emit.c
+++ b/src/intel/compiler/brw_eu_emit.c
@@ -824,6 +824,18 @@ brw_alu3(struct brw_codegen *p, unsigned opcode, struct brw_reg dest,
dest.type == BRW_REGISTER_TYPE_DF ||
dest.type == BRW_REGISTER_TYPE_D ||
dest.type == BRW_REGISTER_TYPE_UD);
+
+ /* Flag registers are in a different spot on 3src instructions so we
+ * need to move the value if we want brw_set_default_flag_reg to work
+ * properly.
+ */
+ unsigned flag_reg_nr =
+ devinfo->gen >= 7 ? brw_inst_flag_reg_nr(devinfo, inst) : 0;
+ unsigned flag_subreg_nr = brw_inst_flag_subreg_nr(devinfo, inst);
+ if (devinfo->gen >= 7)
+ brw_inst_set_3src_a16_flag_reg_nr(devinfo, inst, flag_reg_nr);
+ brw_inst_set_3src_a16_flag_subreg_nr(devinfo, inst, flag_subreg_nr);
+
This seems really gross... brw_next_insn() with a 3-source opcode gives
you a corrupted 3-source instruction initialized as if it were a
2-source one. This fixes the 3-source flag register field hoping that
all other fields of p->current will magically match the 3-source
instruction layout, and hoping that the stray bits of the 2-source flag
register field will be overwritten by something else in this function.

Proper fix would be to get rid of the p->current thing altogether IMO
and use a mechanism to track instruction defaults based on their
semantics rather than on the binary encoding of an instruction which has
unknown encoding... That would be rather invasive though, a compromise
might be to fix it in brw_next_insn() by starting with a clean
instruction and initializing it from scratch with the 3src helpers...
Post by Jason Ekstrand
if (devinfo->gen == 6) {
brw_inst_set_3src_a16_dst_reg_file(devinfo, inst,
dest.file == BRW_MESSAGE_REGISTER_FILE);
--
2.5.0.400.gff86faf
_______________________________________________
mesa-stable mailing list
https://lists.freedesktop.org/mailman/listinfo/mesa-stable
Jason Ekstrand
2018-05-23 20:54:32 UTC
Permalink
Post by Jason Ekstrand
Post by Jason Ekstrand
Prior to gen8, the flag reg and subreg numbers are in different
locations on 3src instructions than on smaller instructions. In order
for brw_set_default_flag_reg to work properly, we need to copy the value
out of the 2src location and write it into the 3src location as part of
brw_alu3.
---
src/intel/compiler/brw_eu_emit.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/src/intel/compiler/brw_eu_emit.c
b/src/intel/compiler/brw_eu_emit.c
Post by Jason Ekstrand
index 4f51d51..fc39d94 100644
--- a/src/intel/compiler/brw_eu_emit.c
+++ b/src/intel/compiler/brw_eu_emit.c
@@ -824,6 +824,18 @@ brw_alu3(struct brw_codegen *p, unsigned opcode,
struct brw_reg dest,
Post by Jason Ekstrand
dest.type == BRW_REGISTER_TYPE_DF ||
dest.type == BRW_REGISTER_TYPE_D ||
dest.type == BRW_REGISTER_TYPE_UD);
+
+ /* Flag registers are in a different spot on 3src instructions so
we
Post by Jason Ekstrand
+ * need to move the value if we want brw_set_default_flag_reg to
work
Post by Jason Ekstrand
+ * properly.
+ */
+ unsigned flag_reg_nr =
+ devinfo->gen >= 7 ? brw_inst_flag_reg_nr(devinfo, inst) : 0;
+ unsigned flag_subreg_nr = brw_inst_flag_subreg_nr(devinfo, inst);
+ if (devinfo->gen >= 7)
+ brw_inst_set_3src_a16_flag_reg_nr(devinfo, inst, flag_reg_nr);
+ brw_inst_set_3src_a16_flag_subreg_nr(devinfo, inst,
flag_subreg_nr);
Post by Jason Ekstrand
+
This seems really gross...
Yes, yes it is. The grossness is not lost on me.
Post by Jason Ekstrand
brw_next_insn() with a 3-source opcode gives
you a corrupted 3-source instruction initialized as if it were a
2-source one. This fixes the 3-source flag register field hoping that
all other fields of p->current will magically match the 3-source
instruction layout, and hoping that the stray bits of the 2-source flag
register field will be overwritten by something else in this function.
Yes, it does. However, I did go through and check and I believe that the
flag [sub]reg number field is the only field that actually moves. Of
course, this is not something we can guarantee going forward, but I think
we're ok today.
Post by Jason Ekstrand
Proper fix would be to get rid of the p->current thing altogether IMO
and use a mechanism to track instruction defaults based on their
semantics rather than on the binary encoding of an instruction which has
unknown encoding...
Agreed. It really bothered me when I found this bug and I think we would
benefit significantly from having a semantic stack rather than this
"default instruction" concept.
Post by Jason Ekstrand
That would be rather invasive though, a compromise
might be to fix it in brw_next_insn() by starting with a clean
instruction and initializing it from scratch with the 3src helpers...
Yes, fixing it in brw_next_insn() might be a better place. Given that all
the other fields match up, I'm a bit inclined to just move this fix to
brw_next_insn() for now and plan to follow up with a logical instruction
control stack. Would that be ok?
Post by Jason Ekstrand
Post by Jason Ekstrand
if (devinfo->gen == 6) {
brw_inst_set_3src_a16_dst_reg_file(devinfo, inst,
dest.file ==
BRW_MESSAGE_REGISTER_FILE);
Post by Jason Ekstrand
--
2.5.0.400.gff86faf
_______________________________________________
mesa-stable mailing list
https://lists.freedesktop.org/mailman/listinfo/mesa-stable
Francisco Jerez
2018-05-23 22:40:57 UTC
Permalink
Post by Jason Ekstrand
Post by Jason Ekstrand
Post by Jason Ekstrand
Prior to gen8, the flag reg and subreg numbers are in different
locations on 3src instructions than on smaller instructions. In order
for brw_set_default_flag_reg to work properly, we need to copy the value
out of the 2src location and write it into the 3src location as part of
brw_alu3.
---
src/intel/compiler/brw_eu_emit.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/src/intel/compiler/brw_eu_emit.c
b/src/intel/compiler/brw_eu_emit.c
Post by Jason Ekstrand
index 4f51d51..fc39d94 100644
--- a/src/intel/compiler/brw_eu_emit.c
+++ b/src/intel/compiler/brw_eu_emit.c
@@ -824,6 +824,18 @@ brw_alu3(struct brw_codegen *p, unsigned opcode,
struct brw_reg dest,
Post by Jason Ekstrand
dest.type == BRW_REGISTER_TYPE_DF ||
dest.type == BRW_REGISTER_TYPE_D ||
dest.type == BRW_REGISTER_TYPE_UD);
+
+ /* Flag registers are in a different spot on 3src instructions so
we
Post by Jason Ekstrand
+ * need to move the value if we want brw_set_default_flag_reg to
work
Post by Jason Ekstrand
+ * properly.
+ */
+ unsigned flag_reg_nr =
+ devinfo->gen >= 7 ? brw_inst_flag_reg_nr(devinfo, inst) : 0;
+ unsigned flag_subreg_nr = brw_inst_flag_subreg_nr(devinfo, inst);
+ if (devinfo->gen >= 7)
+ brw_inst_set_3src_a16_flag_reg_nr(devinfo, inst, flag_reg_nr);
+ brw_inst_set_3src_a16_flag_subreg_nr(devinfo, inst,
flag_subreg_nr);
Post by Jason Ekstrand
+
This seems really gross...
Yes, yes it is. The grossness is not lost on me.
Post by Jason Ekstrand
brw_next_insn() with a 3-source opcode gives
you a corrupted 3-source instruction initialized as if it were a
2-source one. This fixes the 3-source flag register field hoping that
all other fields of p->current will magically match the 3-source
instruction layout, and hoping that the stray bits of the 2-source flag
register field will be overwritten by something else in this function.
Yes, it does. However, I did go through and check and I believe that the
flag [sub]reg number field is the only field that actually moves. Of
course, this is not something we can guarantee going forward, but I think
we're ok today.
Post by Jason Ekstrand
Proper fix would be to get rid of the p->current thing altogether IMO
and use a mechanism to track instruction defaults based on their
semantics rather than on the binary encoding of an instruction which has
unknown encoding...
Agreed. It really bothered me when I found this bug and I think we would
benefit significantly from having a semantic stack rather than this
"default instruction" concept.
Post by Jason Ekstrand
That would be rather invasive though, a compromise
might be to fix it in brw_next_insn() by starting with a clean
instruction and initializing it from scratch with the 3src helpers...
Yes, fixing it in brw_next_insn() might be a better place. Given that all
the other fields match up, I'm a bit inclined to just move this fix to
brw_next_insn() for now and plan to follow up with a logical instruction
control stack. Would that be ok?
Yeah, I suppose, but this patch still only fixes half of the problem,
there are still bogus bits set on the 3-source instruction by the memcpy
from p->current, which we rely on other code overwriting for
correctness, which seems extremely fragile (assuming it's working at all
for all codepaths below). It would be better (and probably
straightforward enough for it to make it to stable releases) to
initialize the new instruction to zero in brw_next_insn() and initialize
the instruction fields manually by using the 3src helpers in case we get
a 3-source opcode.
Post by Jason Ekstrand
Post by Jason Ekstrand
Post by Jason Ekstrand
if (devinfo->gen == 6) {
brw_inst_set_3src_a16_dst_reg_file(devinfo, inst,
dest.file ==
BRW_MESSAGE_REGISTER_FILE);
Post by Jason Ekstrand
--
2.5.0.400.gff86faf
_______________________________________________
mesa-stable mailing list
https://lists.freedesktop.org/mailman/listinfo/mesa-stable
Jason Ekstrand
2018-05-23 23:04:18 UTC
Permalink
Post by Jason Ekstrand
Post by Jason Ekstrand
Post by Jason Ekstrand
Post by Jason Ekstrand
Prior to gen8, the flag reg and subreg numbers are in different
locations on 3src instructions than on smaller instructions. In order
for brw_set_default_flag_reg to work properly, we need to copy the
value
Post by Jason Ekstrand
Post by Jason Ekstrand
Post by Jason Ekstrand
out of the 2src location and write it into the 3src location as part
of
Post by Jason Ekstrand
Post by Jason Ekstrand
Post by Jason Ekstrand
brw_alu3.
---
src/intel/compiler/brw_eu_emit.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/src/intel/compiler/brw_eu_emit.c
b/src/intel/compiler/brw_eu_emit.c
Post by Jason Ekstrand
index 4f51d51..fc39d94 100644
--- a/src/intel/compiler/brw_eu_emit.c
+++ b/src/intel/compiler/brw_eu_emit.c
@@ -824,6 +824,18 @@ brw_alu3(struct brw_codegen *p, unsigned opcode,
struct brw_reg dest,
Post by Jason Ekstrand
dest.type == BRW_REGISTER_TYPE_DF ||
dest.type == BRW_REGISTER_TYPE_D ||
dest.type == BRW_REGISTER_TYPE_UD);
+
+ /* Flag registers are in a different spot on 3src instructions
so
Post by Jason Ekstrand
Post by Jason Ekstrand
we
Post by Jason Ekstrand
+ * need to move the value if we want brw_set_default_flag_reg
to
Post by Jason Ekstrand
Post by Jason Ekstrand
work
Post by Jason Ekstrand
+ * properly.
+ */
+ unsigned flag_reg_nr =
+ devinfo->gen >= 7 ? brw_inst_flag_reg_nr(devinfo, inst) : 0;
+ unsigned flag_subreg_nr = brw_inst_flag_subreg_nr(devinfo,
inst);
Post by Jason Ekstrand
Post by Jason Ekstrand
Post by Jason Ekstrand
+ if (devinfo->gen >= 7)
+ brw_inst_set_3src_a16_flag_reg_nr(devinfo, inst,
flag_reg_nr);
Post by Jason Ekstrand
Post by Jason Ekstrand
Post by Jason Ekstrand
+ brw_inst_set_3src_a16_flag_subreg_nr(devinfo, inst,
flag_subreg_nr);
Post by Jason Ekstrand
+
This seems really gross...
Yes, yes it is. The grossness is not lost on me.
Post by Jason Ekstrand
brw_next_insn() with a 3-source opcode gives
you a corrupted 3-source instruction initialized as if it were a
2-source one. This fixes the 3-source flag register field hoping that
all other fields of p->current will magically match the 3-source
instruction layout, and hoping that the stray bits of the 2-source flag
register field will be overwritten by something else in this function.
Yes, it does. However, I did go through and check and I believe that the
flag [sub]reg number field is the only field that actually moves. Of
course, this is not something we can guarantee going forward, but I think
we're ok today.
Post by Jason Ekstrand
Proper fix would be to get rid of the p->current thing altogether IMO
and use a mechanism to track instruction defaults based on their
semantics rather than on the binary encoding of an instruction which has
unknown encoding...
Agreed. It really bothered me when I found this bug and I think we would
benefit significantly from having a semantic stack rather than this
"default instruction" concept.
Post by Jason Ekstrand
That would be rather invasive though, a compromise
might be to fix it in brw_next_insn() by starting with a clean
instruction and initializing it from scratch with the 3src helpers...
Yes, fixing it in brw_next_insn() might be a better place. Given that
all
Post by Jason Ekstrand
the other fields match up, I'm a bit inclined to just move this fix to
brw_next_insn() for now and plan to follow up with a logical instruction
control stack. Would that be ok?
Yeah, I suppose, but this patch still only fixes half of the problem,
there are still bogus bits set on the 3-source instruction by the memcpy
from p->current, which we rely on other code overwriting for
correctness, which seems extremely fragile (assuming it's working at all
for all codepaths below). It would be better (and probably
straightforward enough for it to make it to stable releases) to
initialize the new instruction to zero in brw_next_insn() and initialize
the instruction fields manually by using the 3src helpers in case we get
a 3-source opcode.
Yeah. I hadn't thought about other bits carying over. :( I'll see what we
I can do about just getting rid of the memcpy for 3src instructions.
Post by Jason Ekstrand
Post by Jason Ekstrand
Post by Jason Ekstrand
Post by Jason Ekstrand
if (devinfo->gen == 6) {
brw_inst_set_3src_a16_dst_reg_file(devinfo, inst,
dest.file ==
BRW_MESSAGE_REGISTER_FILE);
Post by Jason Ekstrand
--
2.5.0.400.gff86faf
_______________________________________________
mesa-stable mailing list
https://lists.freedesktop.org/mailman/listinfo/mesa-stable
Loading...