[Mesa-dev] [PATCH 1/3] i965/fs: Implement the WaCMPInstFlagDepClearedEarly work-around.

[Kenneth Graunke]

On Tuesday, February 03, 2015 10:17:36 PM Matt Turner wrote:
> Prevents piglit regressions from the next patch.
> ---
>  src/mesa/drivers/dri/i965/brw_fs_generator.cpp | 37 +++++++++++++++++++++++++-
>  1 file changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
> index 77d4908..8cd36f8 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
> @@ -1734,7 +1734,42 @@ fs_generator::generate_code(const cfg_t *cfg, int dispatch_width)
>           brw_F16TO32(p, dst, src[0]);
>           break;
>        case BRW_OPCODE_CMP:
> -	 brw_CMP(p, dst, inst->conditional_mod, src[0], src[1]);
> +         /* The Ivybridge/BayTrail WaCMPInstFlagDepClearedEarly workaround says
> +          * that when the destination is a GRF that the dependency-clear bit on
> +          * the flag register is cleared early.
> +          *
> +          * Suggested workarounds are to disable coissuing CMP instructions
> +          * or to split CMP(16) instructions into two CMP(8) instructions.
> +          *
> +          * We choose to split into CMP(8) instructions since disabling
> +          * coissuing would affect CMP instructions not otherwise affected by
> +          * the errata.
> +          */
> +         if (dispatch_width == 16 && brw->gen == 7 && !brw->is_haswell) {
> +            if (dst.file == BRW_GENERAL_REGISTER_FILE) {
> +               brw_set_default_compression_control(p, BRW_COMPRESSION_NONE);
> +               brw_CMP(p, firsthalf(dst), inst->conditional_mod,
> +                          firsthalf(src[0]), firsthalf(src[1]));
> +               brw_set_default_compression_control(p, BRW_COMPRESSION_2NDHALF);
> +               brw_CMP(p, sechalf(dst), inst->conditional_mod,
> +                          sechalf(src[0]), sechalf(src[1]));
> +               brw_set_default_compression_control(p, BRW_COMPRESSION_COMPRESSED);
> +
> +               multiple_instructions_emitted = true;
> +            } else if (dst.file == BRW_ARCHITECTURE_REGISTER_FILE) {
> +               /* For unknown reasons, the aforementioned workaround is not
> +                * sufficient. Overriding the type when the destination is the
> +                * null register is necessary but not sufficient by itself.
> +                */
> +               assert(dst.nr == BRW_ARF_NULL);
> +               dst.type = BRW_REGISTER_TYPE_D;
> +               brw_CMP(p, dst, inst->conditional_mod, src[0], src[1]);
> +            } else {
> +               unreachable("not reached");
> +            }
> +         } else {
> +            brw_CMP(p, dst, inst->conditional_mod, src[0], src[1]);
> +         }
>  	 break;
>        case BRW_OPCODE_SEL:
>  	 brw_SEL(p, dst, src[0], src[1]);
> 

These three patches seem reasonable to me.
Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>

I do wonder, though - overriding the destination type to D effectively
disables coissuing for a single instruction.  I know the documentation
claims that source types = F and destination types = D isn't legal, but
we've been doing it for ages with no apparent issues.  This might be a
less expensive workaround.

Then again, if it were that easy, presumably the other driver team
would've done it that way...
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150204/cbbd7c2b/attachment.sig>