Re: [Mesa-dev] [PATCH 05/20] i965/fs: Handle explicit flag destinations in flags_written()
On Fri, Jul 14, 2017 at 3:23 PM, Francisco Jerez wrote: > Matt Turner writes: > >> The implementations of the ARB_shader_group_vote intrinsics will >> explicitly write the flag as the destination register. >> --- >> src/intel/compiler/brw_fs.cpp | 12 ++-- >> 1 file changed, 10 insertions(+), 2 deletions(-) >> >> diff --git a/src/intel/compiler/brw_fs.cpp b/src/intel/compiler/brw_fs.cpp >> index 43b6e34204..97908a4563 100644 >> --- a/src/intel/compiler/brw_fs.cpp >> +++ b/src/intel/compiler/brw_fs.cpp >> @@ -890,9 +890,17 @@ fs_inst::flags_written() const >> opcode != BRW_OPCODE_WHILE)) || >> opcode == FS_OPCODE_MOV_DISPATCH_TO_FLAGS) { >>return flag_mask(this); >> - } else { >> - return 0; >> + } else if (dst.file == ARF) { >> + if (dst.nr == BRW_ARF_FLAG + 0 && dst.subnr == 0) >> + return 0b0001; >> + if (dst.nr == BRW_ARF_FLAG + 0 && dst.subnr == 1) >> + return 0b0010; >> + if (dst.nr == BRW_ARF_FLAG + 1 && dst.subnr == 0) >> + return 0b0100; >> + if (dst.nr == BRW_ARF_FLAG + 1 && dst.subnr == 1) >> + return 0b1000; > > This doesn't look right to me, it assumes that flag registers are 16b > (they're 32b), and that you can only write a single byte of the flag > register at a time, which is hardly ever the case. I don't think I read the implementation of flag_mask(), just the comment, and thought that it returned a bit per flag subregister. > > I think you want something along the lines of: > > | unsigned > | bit_mask(unsigned n) > | { > |return (n >= CHAR_BIT * sizeof(bit_mask(n)) ? ~0u : (1 << n) - 1); > | } > | > | unsigned > | flag_mask(const fs_reg &r, unsigned sz) > | { > |if (r.file == ARF) { > | const unsigned start = (r.nr - BRW_ARF_FLAG) * 4 + r.subnr; > | const unsigned end = start + sz; > | return bit_mask(end) & ~bit_mask(start); > |} else { > | return 0; > |} > | } > > [Yes, this should give you the right result (i.e. zero) for ARF > registers outside the range of the (flag) ARF window of the 32-bit > bitmask.] Notice the similarity between this and the current > flag_mask() overload. This will allow you to easily re-use the same > logic for both flags_read() and flags_written(), you just need to pass > 'inst->src[i], inst->size_read(i)' and 'inst->dst, inst->size_written' > as arguments respectively, ORing the results for each source in the > former case. Wow, thanks. That's fantastic. I've made this change (and also changed the authorship on 05/20 and 12/20 to you! :) ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 05/20] i965/fs: Handle explicit flag destinations in flags_written()
Matt Turner writes: > The implementations of the ARB_shader_group_vote intrinsics will > explicitly write the flag as the destination register. > --- > src/intel/compiler/brw_fs.cpp | 12 ++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/src/intel/compiler/brw_fs.cpp b/src/intel/compiler/brw_fs.cpp > index 43b6e34204..97908a4563 100644 > --- a/src/intel/compiler/brw_fs.cpp > +++ b/src/intel/compiler/brw_fs.cpp > @@ -890,9 +890,17 @@ fs_inst::flags_written() const > opcode != BRW_OPCODE_WHILE)) || > opcode == FS_OPCODE_MOV_DISPATCH_TO_FLAGS) { >return flag_mask(this); > - } else { > - return 0; > + } else if (dst.file == ARF) { > + if (dst.nr == BRW_ARF_FLAG + 0 && dst.subnr == 0) > + return 0b0001; > + if (dst.nr == BRW_ARF_FLAG + 0 && dst.subnr == 1) > + return 0b0010; > + if (dst.nr == BRW_ARF_FLAG + 1 && dst.subnr == 0) > + return 0b0100; > + if (dst.nr == BRW_ARF_FLAG + 1 && dst.subnr == 1) > + return 0b1000; This doesn't look right to me, it assumes that flag registers are 16b (they're 32b), and that you can only write a single byte of the flag register at a time, which is hardly ever the case. I think you want something along the lines of: | unsigned | bit_mask(unsigned n) | { |return (n >= CHAR_BIT * sizeof(bit_mask(n)) ? ~0u : (1 << n) - 1); | } | | unsigned | flag_mask(const fs_reg &r, unsigned sz) | { |if (r.file == ARF) { | const unsigned start = (r.nr - BRW_ARF_FLAG) * 4 + r.subnr; | const unsigned end = start + sz; | return bit_mask(end) & ~bit_mask(start); |} else { | return 0; |} | } [Yes, this should give you the right result (i.e. zero) for ARF registers outside the range of the (flag) ARF window of the 32-bit bitmask.] Notice the similarity between this and the current flag_mask() overload. This will allow you to easily re-use the same logic for both flags_read() and flags_written(), you just need to pass 'inst->src[i], inst->size_read(i)' and 'inst->dst, inst->size_written' as arguments respectively, ORing the results for each source in the former case. Also make sure to remove the 'XXX' comments at the top of both functions saying that explicit flag reads and writes are not handled. ;) > } > + return 0; > } > > /** > -- > 2.13.0 > > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 05/20] i965/fs: Handle explicit flag destinations in flags_written()
The implementations of the ARB_shader_group_vote intrinsics will explicitly write the flag as the destination register. --- src/intel/compiler/brw_fs.cpp | 12 ++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/intel/compiler/brw_fs.cpp b/src/intel/compiler/brw_fs.cpp index 43b6e34204..97908a4563 100644 --- a/src/intel/compiler/brw_fs.cpp +++ b/src/intel/compiler/brw_fs.cpp @@ -890,9 +890,17 @@ fs_inst::flags_written() const opcode != BRW_OPCODE_WHILE)) || opcode == FS_OPCODE_MOV_DISPATCH_TO_FLAGS) { return flag_mask(this); - } else { - return 0; + } else if (dst.file == ARF) { + if (dst.nr == BRW_ARF_FLAG + 0 && dst.subnr == 0) + return 0b0001; + if (dst.nr == BRW_ARF_FLAG + 0 && dst.subnr == 1) + return 0b0010; + if (dst.nr == BRW_ARF_FLAG + 1 && dst.subnr == 0) + return 0b0100; + if (dst.nr == BRW_ARF_FLAG + 1 && dst.subnr == 1) + return 0b1000; } + return 0; } /** -- 2.13.0 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev