[Mesa-dev] [PATCH 20/24] Revert "i965: Have brw_imm_vf4() take the vector components as integer values."

[Francisco Jerez]

Matt Turner <mattst88 at gmail.com> writes:

> This reverts commit bbf8239f92ecd79431dfa41402e1c85318e7267f.
>
> I didn't like that commit to begin with -- computing things at compile
> time is fine -- but for purposes of verifying that the resulting values
> are correct, looking up 0x00 and 0x30 in a table is a lot better than
> evaluating a recursive function.
>
FYI the function only ever recurses once in order to handle the sign bit
orthogonally from the exponent and mantissa.

> Anyway, by making brw_imm_vf4() take the actual 8-bit restricted floats
> directly (instead of only integral values that would be converted to
> restricted float), we can use this function as a replacement for the
> vector float src_reg/fs_reg constructors.

Seems awful to me, it replaces a formula that can be verified correct by
reading the first paragraph of the description of the restricted float
format (Gen Graphics » BSpec » 3D-Media-GPGPU Engine » EU Overview » ISA
Introduction » EU Data Types » Numeric Data Types) with a table of magic
constants that don't give the slightest insight into the structure of
the restricted float format and therefore have to be verified
individually.

It also makes it impossible to use brw_imm_vf4() with
dynamically-specified integer constants which was the original
motivation of this change.

If the obfuscation is meant as an optimization, I don't think it helps.
GCC compiles the four int_to_float8() calls from brw_imm_vf4() into a
single instruction already when the arguments are constant expressions:

  4a9dc9:       48 b8 00 00 00 00 30    movabs $0x3000000000,%rax

(assembly taken from brw_clip_interp_vertex())

> ---
>  src/mesa/drivers/dri/i965/brw_clip_util.c |  2 +-
>  src/mesa/drivers/dri/i965/brw_reg.h       | 40 ++++++++-----------------------
>  2 files changed, 11 insertions(+), 31 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_clip_util.c b/src/mesa/drivers/dri/i965/brw_clip_util.c
> index 40ad144..0253d52 100644
> --- a/src/mesa/drivers/dri/i965/brw_clip_util.c
> +++ b/src/mesa/drivers/dri/i965/brw_clip_util.c
> @@ -224,7 +224,7 @@ void brw_clip_interp_vertex( struct brw_clip_compile *c,
>                vec1(t_nopersp),
>                brw_imm_f(0));
>        brw_IF(p, BRW_EXECUTE_1);
> -      brw_MOV(p, t_nopersp, brw_imm_vf4(1, 0, 0, 0));
> +      brw_MOV(p, t_nopersp, brw_imm_vf4(VF_ONE, VF_ZERO, VF_ZERO, VF_ZERO));
>        brw_ENDIF(p);
>  
>        /* Now compute t_nopersp = t_nopersp.y/t_nopersp.x and broadcast it. */
> diff --git a/src/mesa/drivers/dri/i965/brw_reg.h b/src/mesa/drivers/dri/i965/brw_reg.h
> index b906892..a4c1901 100644
> --- a/src/mesa/drivers/dri/i965/brw_reg.h
> +++ b/src/mesa/drivers/dri/i965/brw_reg.h
> @@ -43,7 +43,6 @@
>  #define BRW_REG_H
>  
>  #include <stdbool.h>
> -#include "main/imports.h"
>  #include "main/compiler.h"
>  #include "main/macros.h"
>  #include "program/prog_instruction.h"
> @@ -638,38 +637,19 @@ brw_imm_vf(unsigned v)
>     return imm;
>  }
>  
> -/**
> - * Convert an integer into a "restricted" 8-bit float, used in vector
> - * immediates.  The 8-bit floating point format has a sign bit, an
> - * excess-3 3-bit exponent, and a 4-bit mantissa.  All integer values
> - * from -31 to 31 can be represented exactly.
> - */
> -static inline uint8_t
> -int_to_float8(int x)
> -{
> -   if (x == 0) {
> -      return 0;
> -   } else if (x < 0) {
> -      return 1 << 7 | int_to_float8(-x);
> -   } else {
> -      const unsigned exponent = _mesa_logbase2(x);
> -      const unsigned mantissa = (x - (1 << exponent)) << (4 - exponent);
> -      assert(exponent <= 4);
> -      return (exponent + 3) << 4 | mantissa;
> -   }
> -}
> +#define VF_ZERO 0x0
> +#define VF_ONE  0x30
> +#define VF_NEG  (1<<7)
>  
> -/**
> - * Construct a floating-point packed vector immediate from its integer
> - * values. \sa int_to_float8()
> - */
>  static inline struct brw_reg
> -brw_imm_vf4(int v0, int v1, int v2, int v3)
> +brw_imm_vf4(unsigned v0, unsigned v1, unsigned v2, unsigned v3)
>  {
> -   return brw_imm_vf((int_to_float8(v0) << 0) |
> -                     (int_to_float8(v1) << 8) |
> -                     (int_to_float8(v2) << 16) |
> -                     (int_to_float8(v3) << 24));
> +   struct brw_reg imm = brw_imm_reg(BRW_REGISTER_TYPE_VF);
> +   imm.vstride = BRW_VERTICAL_STRIDE_0;
> +   imm.width = BRW_WIDTH_4;
> +   imm.hstride = BRW_HORIZONTAL_STRIDE_1;
> +   imm.ud = ((v0 << 0) | (v1 << 8) | (v2 << 16) | (v3 << 24));
> +   return imm;
>  }
>  
>  
> -- 
> 2.4.9
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20151103/ae6858ac/attachment.sig>