[Mesa-dev] [PATCH v2] i965: Refactor SIMD16-to-2xSIMD8 checks.

[Matt Turner]

On Thu, Mar 19, 2015 at 11:43 AM, Neil Roberts <neil at linux.intel.com> wrote:
> The places that were checking whether 3-source instructions are
> supported have now been combined into a small helper function. This
> will be used in the next patch to add an additonal restriction.
>
> Based on a patch by Kenneth Graunke.
>
> ---
>
> Matt's review for v1 of this patch was conditional based on moving the
> call to brw_instruction_supports_simd16 into the switch statement. I
> went ahead and made this change in order to try and unblock this
> patch. However if we are only calling this function inside the case
> values I think it no longer makes sense to have another switch
> on the opcode inside the new function. Instead I've changed the
> function to just handle deciding whether 3-source SIMD16 instructions
> are allowed and left the previous per-instruction specifics about
> whether general SIMD16 is allowed as they were. I think it is a bit
> cleaner this way.
>
> I also noticed that v1 of the patch didn't have a case for the BFE
> instruction so it wouldn't be broken down. I think that is a good
> indication that it's better to keep the handling for each instruction
> in a single place.
>
>  src/mesa/drivers/dri/i965/brw_fs_generator.cpp | 9 +++++----
>  src/mesa/drivers/dri/i965/brw_shader.cpp       | 9 +++++++++
>  src/mesa/drivers/dri/i965/brw_shader.h         | 1 +
>  3 files changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
> index 05a2db4..6cf7811 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
> @@ -1646,7 +1646,7 @@ fs_generator::generate_code(const cfg_t *cfg, int dispatch_width)
>        case BRW_OPCODE_MAD:
>           assert(brw->gen >= 6);
>          brw_set_default_access_mode(p, BRW_ALIGN_16);
> -         if (dispatch_width == 16 && brw->gen < 8 && !brw->is_haswell) {
> +         if (dispatch_width == 16 && !brw_supports_simd16_3src(brw)) {
>             brw_set_default_compression_control(p, BRW_COMPRESSION_NONE);
>              brw_inst *f = brw_MAD(p, firsthalf(dst), firsthalf(src[0]), firsthalf(src[1]), firsthalf(src[2]));
>             brw_set_default_compression_control(p, BRW_COMPRESSION_2NDHALF);
> @@ -1667,7 +1667,7 @@ fs_generator::generate_code(const cfg_t *cfg, int dispatch_width)
>        case BRW_OPCODE_LRP:
>           assert(brw->gen >= 6);
>          brw_set_default_access_mode(p, BRW_ALIGN_16);
> -         if (dispatch_width == 16 && brw->gen < 8 && !brw->is_haswell) {
> +         if (dispatch_width == 16 && !brw_supports_simd16_3src(brw)) {
>             brw_set_default_compression_control(p, BRW_COMPRESSION_NONE);
>              brw_inst *f = brw_LRP(p, firsthalf(dst), firsthalf(src[0]), firsthalf(src[1]), firsthalf(src[2]));
>             brw_set_default_compression_control(p, BRW_COMPRESSION_2NDHALF);
> @@ -1804,7 +1804,7 @@ fs_generator::generate_code(const cfg_t *cfg, int dispatch_width)
>        case BRW_OPCODE_BFE:
>           assert(brw->gen >= 7);
>           brw_set_default_access_mode(p, BRW_ALIGN_16);
> -         if (dispatch_width == 16 && brw->gen < 8 && !brw->is_haswell) {
> +         if (dispatch_width == 16 && !brw_supports_simd16_3src(brw)) {
>              brw_set_default_compression_control(p, BRW_COMPRESSION_NONE);
>              brw_BFE(p, firsthalf(dst), firsthalf(src[0]), firsthalf(src[1]), firsthalf(src[2]));
>              brw_set_default_compression_control(p, BRW_COMPRESSION_2NDHALF);
> @@ -1844,7 +1844,8 @@ fs_generator::generate_code(const cfg_t *cfg, int dispatch_width)
>            * Otherwise we would be able to emit compressed instructions like we
>            * do for the other three-source instructions.
>            */
> -         if (dispatch_width == 16 && brw->gen < 8) {
> +         if (dispatch_width == 16 &&
> +             (brw->is_haswell || !brw_supports_simd16_3src(brw))) {
>              brw_set_default_compression_control(p, BRW_COMPRESSION_NONE);
>              brw_BFI2(p, firsthalf(dst), firsthalf(src[0]), firsthalf(src[1]), firsthalf(src[2]));
>              brw_set_default_compression_control(p, BRW_COMPRESSION_2NDHALF);
> diff --git a/src/mesa/drivers/dri/i965/brw_shader.cpp b/src/mesa/drivers/dri/i965/brw_shader.cpp
> index 51c965c..fa1394a 100644
> --- a/src/mesa/drivers/dri/i965/brw_shader.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_shader.cpp
> @@ -390,6 +390,15 @@ brw_texture_offset(struct gl_context *ctx, int *offsets,
>     return offset_bits;
>  }
>
> +/**
> + * Some hardware doesn't support SIMD16 instructions with 3 sources.
> + */
> +bool
> +brw_supports_simd16_3src(const struct brw_context *brw)
> +{
> +   return brw->is_haswell || brw->gen >= 8;
> +}

I don't see this being useful outside of brw_fs_generator.cpp. I'd
move it there and make it static.

With that,

Reviewed-by: Matt Turner <mattst88 at gmail.com>