Re: [Mesa-dev] [PATCH 09/12] i965/fs: Hook up SIMD lowering to handle texturing opcodes of unsupported width.
On Saturday, July 18, 2015 05:34:55 PM Francisco Jerez wrote: This should match the set of cases in which we currently call fail() or no16() from the emit_texture_*() methods and the ones in which emit_texture_gen4() enables the SIMD16 workaround. Hint for reviewers: It's not a big deal if I happen to have missed some case here, it will just lead to an assertion failure down the road which is easily fixable, however being stricter than necessary won't cause any visible breakage, it would just decrease performance silently due to the unnecessary message splitting, so feel free to double-check that all cases listed here already cause a SIMD8/16 fall-back with the current texturing code -- You may want to skip over the Gen5-6 cases though if you don't have pencil and paper at hand. --- src/mesa/drivers/dri/i965/brw_fs.cpp | 27 +++ 1 file changed, 27 insertions(+) diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp index 043d9e9..f291202 100644 --- a/src/mesa/drivers/dri/i965/brw_fs.cpp +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp @@ -3918,6 +3918,33 @@ get_lowered_simd_width(const struct brw_device_info *devinfo, /* Dual-source FB writes are unsupported in SIMD16 mode. */ return (inst-src[1].file != BAD_FILE ? 8 : inst-exec_size); + case SHADER_OPCODE_TXD_LOGICAL: + /* TXD is unsupported in SIMD16 mode. */ + return 8; + + case SHADER_OPCODE_TG4_OFFSET_LOGICAL: { + /* gather4_po_c is unsupported in SIMD16 mode. */ + const fs_reg shadow_c = inst-src[1]; + return (shadow_c.file != BAD_FILE ? 8 : inst-exec_size); + } + case SHADER_OPCODE_TXL_LOGICAL: + case FS_OPCODE_TXB_LOGICAL: { + /* Gen4 doesn't have SIMD8 non-shadow-compare bias/LOD instructions, and + * Gen4-6 don't support TXL and TXB with shadow comparison in SIMD16 + * mode. + */ I agree with Jason here, I think this would be a lot easier to read if we separated the gen == 4 and gen = 6 cases into two if statements. (I do like pure expressions in general, though!) While you're doing that, it would be great to say /* Gen4-6 can't support TXL and TXB with shadow comparison in SIMD16 * mode because the message exceeds the maximum length of 11. */ (it wasn't obvious to me why that was disallowed, at first) Other than my concern about regs_read() for offset in patch 1, everything looks great to me. With that sorted out, this series is: Reviewed-by: Kenneth Graunke kenn...@whitecape.org Thanks Curro! signature.asc Description: This is a digitally signed message part. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 09/12] i965/fs: Hook up SIMD lowering to handle texturing opcodes of unsupported width.
On Wed, Jul 22, 2015 at 12:55 AM, Francisco Jerez curroje...@riseup.net wrote: Jason Ekstrand ja...@jlekstrand.net writes: On Sat, Jul 18, 2015 at 7:34 AM, Francisco Jerez curroje...@riseup.net wrote: This should match the set of cases in which we currently call fail() or no16() from the emit_texture_*() methods and the ones in which emit_texture_gen4() enables the SIMD16 workaround. Hint for reviewers: It's not a big deal if I happen to have missed some case here, it will just lead to an assertion failure down the road which is easily fixable, however being stricter than necessary won't cause any visible breakage, it would just decrease performance silently due to the unnecessary message splitting, so feel free to double-check that all cases listed here already cause a SIMD8/16 fall-back with the current texturing code -- You may want to skip over the Gen5-6 cases though if you don't have pencil and paper at hand. I'll believe you. Even if you got it wrong, the worst that happens is we hit the fail case when we try and lower. --- src/mesa/drivers/dri/i965/brw_fs.cpp | 27 +++ 1 file changed, 27 insertions(+) diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp index 043d9e9..f291202 100644 --- a/src/mesa/drivers/dri/i965/brw_fs.cpp +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp @@ -3918,6 +3918,33 @@ get_lowered_simd_width(const struct brw_device_info *devinfo, /* Dual-source FB writes are unsupported in SIMD16 mode. */ return (inst-src[1].file != BAD_FILE ? 8 : inst-exec_size); + case SHADER_OPCODE_TXD_LOGICAL: + /* TXD is unsupported in SIMD16 mode. */ + return 8; + + case SHADER_OPCODE_TG4_OFFSET_LOGICAL: { + /* gather4_po_c is unsupported in SIMD16 mode. */ + const fs_reg shadow_c = inst-src[1]; + return (shadow_c.file != BAD_FILE ? 8 : inst-exec_size); + } + case SHADER_OPCODE_TXL_LOGICAL: + case FS_OPCODE_TXB_LOGICAL: { + /* Gen4 doesn't have SIMD8 non-shadow-compare bias/LOD instructions, and + * Gen4-6 don't support TXL and TXB with shadow comparison in SIMD16 + * mode. + */ + const fs_reg shadow_c = inst-src[1]; + return (devinfo-gen == 4 shadow_c.file == BAD_FILE ? 16 : + devinfo-gen 7 shadow_c.file != BAD_FILE ? 8 : + inst-exec_size); Could we please use an if-ladder here. Nested ternaries are hard to read and kind of pointless given that we can return multiple places in the if. Heh, I always have the very opposite style itch -- If you can compute something succinctly with a handful of pure expressions why venture into the dangerous realm of statements? Heh... The mathematician in me is tempted to agree with you. However, the programmer in me likes his control-flow to have obvious nesting and indentation. :-) I think John Backus' article Can programming be liberated from the von Neumann style? although slightly outdated elaborates the point pretty well. But sure, if you'd like it better with an if-ladder why not. + } + case SHADER_OPCODE_TXF_LOGICAL: + case SHADER_OPCODE_TXS_LOGICAL: + /* Gen4 doesn't have SIMD8 variants for the RESINFO and LD-with-LOD + * messages. Use SIMD16 instead. + */ + return (devinfo-gen == 4 ? 16 : inst-exec_size); I'd kind of also like this ternary to go, but I'm ok with it if you want to keep it. + default: return inst-exec_size; } -- 2.4.3 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 09/12] i965/fs: Hook up SIMD lowering to handle texturing opcodes of unsupported width.
Jason Ekstrand ja...@jlekstrand.net writes: On Sat, Jul 18, 2015 at 7:34 AM, Francisco Jerez curroje...@riseup.net wrote: This should match the set of cases in which we currently call fail() or no16() from the emit_texture_*() methods and the ones in which emit_texture_gen4() enables the SIMD16 workaround. Hint for reviewers: It's not a big deal if I happen to have missed some case here, it will just lead to an assertion failure down the road which is easily fixable, however being stricter than necessary won't cause any visible breakage, it would just decrease performance silently due to the unnecessary message splitting, so feel free to double-check that all cases listed here already cause a SIMD8/16 fall-back with the current texturing code -- You may want to skip over the Gen5-6 cases though if you don't have pencil and paper at hand. I'll believe you. Even if you got it wrong, the worst that happens is we hit the fail case when we try and lower. --- src/mesa/drivers/dri/i965/brw_fs.cpp | 27 +++ 1 file changed, 27 insertions(+) diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp index 043d9e9..f291202 100644 --- a/src/mesa/drivers/dri/i965/brw_fs.cpp +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp @@ -3918,6 +3918,33 @@ get_lowered_simd_width(const struct brw_device_info *devinfo, /* Dual-source FB writes are unsupported in SIMD16 mode. */ return (inst-src[1].file != BAD_FILE ? 8 : inst-exec_size); + case SHADER_OPCODE_TXD_LOGICAL: + /* TXD is unsupported in SIMD16 mode. */ + return 8; + + case SHADER_OPCODE_TG4_OFFSET_LOGICAL: { + /* gather4_po_c is unsupported in SIMD16 mode. */ + const fs_reg shadow_c = inst-src[1]; + return (shadow_c.file != BAD_FILE ? 8 : inst-exec_size); + } + case SHADER_OPCODE_TXL_LOGICAL: + case FS_OPCODE_TXB_LOGICAL: { + /* Gen4 doesn't have SIMD8 non-shadow-compare bias/LOD instructions, and + * Gen4-6 don't support TXL and TXB with shadow comparison in SIMD16 + * mode. + */ + const fs_reg shadow_c = inst-src[1]; + return (devinfo-gen == 4 shadow_c.file == BAD_FILE ? 16 : + devinfo-gen 7 shadow_c.file != BAD_FILE ? 8 : + inst-exec_size); Could we please use an if-ladder here. Nested ternaries are hard to read and kind of pointless given that we can return multiple places in the if. Heh, I always have the very opposite style itch -- If you can compute something succinctly with a handful of pure expressions why venture into the dangerous realm of statements? I think John Backus' article Can programming be liberated from the von Neumann style? although slightly outdated elaborates the point pretty well. But sure, if you'd like it better with an if-ladder why not. + } + case SHADER_OPCODE_TXF_LOGICAL: + case SHADER_OPCODE_TXS_LOGICAL: + /* Gen4 doesn't have SIMD8 variants for the RESINFO and LD-with-LOD + * messages. Use SIMD16 instead. + */ + return (devinfo-gen == 4 ? 16 : inst-exec_size); I'd kind of also like this ternary to go, but I'm ok with it if you want to keep it. + default: return inst-exec_size; } -- 2.4.3 signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 09/12] i965/fs: Hook up SIMD lowering to handle texturing opcodes of unsupported width.
On Sat, Jul 18, 2015 at 7:34 AM, Francisco Jerez curroje...@riseup.net wrote: This should match the set of cases in which we currently call fail() or no16() from the emit_texture_*() methods and the ones in which emit_texture_gen4() enables the SIMD16 workaround. Hint for reviewers: It's not a big deal if I happen to have missed some case here, it will just lead to an assertion failure down the road which is easily fixable, however being stricter than necessary won't cause any visible breakage, it would just decrease performance silently due to the unnecessary message splitting, so feel free to double-check that all cases listed here already cause a SIMD8/16 fall-back with the current texturing code -- You may want to skip over the Gen5-6 cases though if you don't have pencil and paper at hand. I'll believe you. Even if you got it wrong, the worst that happens is we hit the fail case when we try and lower. --- src/mesa/drivers/dri/i965/brw_fs.cpp | 27 +++ 1 file changed, 27 insertions(+) diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp index 043d9e9..f291202 100644 --- a/src/mesa/drivers/dri/i965/brw_fs.cpp +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp @@ -3918,6 +3918,33 @@ get_lowered_simd_width(const struct brw_device_info *devinfo, /* Dual-source FB writes are unsupported in SIMD16 mode. */ return (inst-src[1].file != BAD_FILE ? 8 : inst-exec_size); + case SHADER_OPCODE_TXD_LOGICAL: + /* TXD is unsupported in SIMD16 mode. */ + return 8; + + case SHADER_OPCODE_TG4_OFFSET_LOGICAL: { + /* gather4_po_c is unsupported in SIMD16 mode. */ + const fs_reg shadow_c = inst-src[1]; + return (shadow_c.file != BAD_FILE ? 8 : inst-exec_size); + } + case SHADER_OPCODE_TXL_LOGICAL: + case FS_OPCODE_TXB_LOGICAL: { + /* Gen4 doesn't have SIMD8 non-shadow-compare bias/LOD instructions, and + * Gen4-6 don't support TXL and TXB with shadow comparison in SIMD16 + * mode. + */ + const fs_reg shadow_c = inst-src[1]; + return (devinfo-gen == 4 shadow_c.file == BAD_FILE ? 16 : + devinfo-gen 7 shadow_c.file != BAD_FILE ? 8 : + inst-exec_size); Could we please use an if-ladder here. Nested ternaries are hard to read and kind of pointless given that we can return multiple places in the if. + } + case SHADER_OPCODE_TXF_LOGICAL: + case SHADER_OPCODE_TXS_LOGICAL: + /* Gen4 doesn't have SIMD8 variants for the RESINFO and LD-with-LOD + * messages. Use SIMD16 instead. + */ + return (devinfo-gen == 4 ? 16 : inst-exec_size); I'd kind of also like this ternary to go, but I'm ok with it if you want to keep it. + default: return inst-exec_size; } -- 2.4.3 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 09/12] i965/fs: Hook up SIMD lowering to handle texturing opcodes of unsupported width.
This should match the set of cases in which we currently call fail() or no16() from the emit_texture_*() methods and the ones in which emit_texture_gen4() enables the SIMD16 workaround. Hint for reviewers: It's not a big deal if I happen to have missed some case here, it will just lead to an assertion failure down the road which is easily fixable, however being stricter than necessary won't cause any visible breakage, it would just decrease performance silently due to the unnecessary message splitting, so feel free to double-check that all cases listed here already cause a SIMD8/16 fall-back with the current texturing code -- You may want to skip over the Gen5-6 cases though if you don't have pencil and paper at hand. --- src/mesa/drivers/dri/i965/brw_fs.cpp | 27 +++ 1 file changed, 27 insertions(+) diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp index 043d9e9..f291202 100644 --- a/src/mesa/drivers/dri/i965/brw_fs.cpp +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp @@ -3918,6 +3918,33 @@ get_lowered_simd_width(const struct brw_device_info *devinfo, /* Dual-source FB writes are unsupported in SIMD16 mode. */ return (inst-src[1].file != BAD_FILE ? 8 : inst-exec_size); + case SHADER_OPCODE_TXD_LOGICAL: + /* TXD is unsupported in SIMD16 mode. */ + return 8; + + case SHADER_OPCODE_TG4_OFFSET_LOGICAL: { + /* gather4_po_c is unsupported in SIMD16 mode. */ + const fs_reg shadow_c = inst-src[1]; + return (shadow_c.file != BAD_FILE ? 8 : inst-exec_size); + } + case SHADER_OPCODE_TXL_LOGICAL: + case FS_OPCODE_TXB_LOGICAL: { + /* Gen4 doesn't have SIMD8 non-shadow-compare bias/LOD instructions, and + * Gen4-6 don't support TXL and TXB with shadow comparison in SIMD16 + * mode. + */ + const fs_reg shadow_c = inst-src[1]; + return (devinfo-gen == 4 shadow_c.file == BAD_FILE ? 16 : + devinfo-gen 7 shadow_c.file != BAD_FILE ? 8 : + inst-exec_size); + } + case SHADER_OPCODE_TXF_LOGICAL: + case SHADER_OPCODE_TXS_LOGICAL: + /* Gen4 doesn't have SIMD8 variants for the RESINFO and LD-with-LOD + * messages. Use SIMD16 instead. + */ + return (devinfo-gen == 4 ? 16 : inst-exec_size); + default: return inst-exec_size; } -- 2.4.3 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev