[Mesa-dev] [PATCH] nv50/ir: Improve Maintainability of Target*::initOpInfo()

[Karol Herbst]

On Sat, Jun 16, 2018 at 12:26 PM, Rhys Perry <pendingchaos02 at gmail.com> wrote:
> This is mainly useful for when one needs to add new opcodes in a painless
> and reliable way.
>
> Signed-off-by: Rhys Perry <pendingchaos02 at gmail.com>
> ---
>  .../drivers/nouveau/codegen/nv50_ir_target_nv50.cpp | 21 ++++++++++++---------
>  .../drivers/nouveau/codegen/nv50_ir_target_nvc0.cpp | 20 +++++++++++---------
>  2 files changed, 23 insertions(+), 18 deletions(-)
>
> diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir_target_nv50.cpp b/src/gallium/drivers/nouveau/codegen/nv50_ir_target_nv50.cpp
> index 83b4102b0a..c4073000aa 100644
> --- a/src/gallium/drivers/nouveau/codegen/nv50_ir_target_nv50.cpp
> +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_target_nv50.cpp
> @@ -111,16 +111,15 @@ void TargetNV50::initOpInfo()
>  {
>     unsigned int i, j;
>
> -   static const uint32_t commutative[(OP_LAST + 31) / 32] =
> +   static const uint32_t commutativeList[] =

please change the type to operation

>     {
> -      // ADD, MUL, MAD, FMA, AND, OR, XOR, MAX, MIN, SET_AND, SET_OR, SET_XOR,
> -      // SET, SELP, SLCT
> -      0x0ce0ca00, 0x0000007e, 0x00000000, 0x00000000
> +      OP_ADD, OP_MUL, OP_MAD, OP_FMA, OP_AND, OP_OR, OP_XOR, OP_MAX, OP_MIN,
> +      OP_SET_AND, OP_SET_OR, OP_SET_XOR, OP_SET, OP_SELP, OP_SLCT
>     };
> -   static const uint32_t shortForm[(OP_LAST + 31) / 32] =
> +   static const uint32_t shortFormList[] =

please change the type to operation

>     {
> -      // MOV, ADD, SUB, MUL, MAD, SAD, RCP, L/PINTERP, TEX, TXF
> -      0x00014e40, 0x00000080, 0x00001260, 0x00000000
> +      OP_MOV, OP_ADD, OP_SUB, OP_MUL, OP_MAD, OP_SAD, OP_RCP, OP_LINTERP,
> +      OP_PINTERP, OP_TEX, OP_TXF
>     };
>     static const operation noDestList[] =
>     {
> @@ -157,12 +156,16 @@ void TargetNV50::initOpInfo()
>
>        opInfo[i].hasDest = 1;
>        opInfo[i].vector = (i >= OP_TEX && i <= OP_TEXCSAA);
> -      opInfo[i].commutative = (commutative[i / 32] >> (i % 32)) & 1;
> +      opInfo[i].commutative = false;
>        opInfo[i].pseudo = (i < OP_MOV);
>        opInfo[i].predicate = !opInfo[i].pseudo;
>        opInfo[i].flow = (i >= OP_BRA && i <= OP_JOIN);
> -      opInfo[i].minEncSize = (shortForm[i / 32] & (1 << (i % 32))) ? 4 : 8;
> +      opInfo[i].minEncSize = 8;
>     }
> +   for (i = 0; i < sizeof(commutativeList) / sizeof(commutativeList[0]); ++i)
> +      opInfo[commutativeList[i]].commutative = true;
> +   for (i = 0; i < sizeof(shortFormList) / sizeof(shortFormList[0]); ++i)
> +      opInfo[shortFormList[i]].minEncSize = 4;

I think we may should use range-based for loops:

for (const operation &op : commutativeList
   opInfo[op].commutative = true;
for (const operation &op : shortFormList)
   opInfo[op].minEncSize = 4;

It depends on C++11, but I think it is time to move forward :) (it
might require some changes to build system files to require C++11 in
Nouveau.)

Anyway it would make code like this so much cleaner and easier to read :)

The code is fine as it is, but I would prefer to use C++11 in such
cases, because it makes sense.

>     for (i = 0; i < sizeof(noDestList) / sizeof(noDestList[0]); ++i)
>        opInfo[noDestList[i]].hasDest = 0;
>     for (i = 0; i < sizeof(noPredList) / sizeof(noPredList[0]); ++i)
> diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir_target_nvc0.cpp b/src/gallium/drivers/nouveau/codegen/nv50_ir_target_nvc0.cpp
> index 954aec0a2f..cc1efb4e71 100644
> --- a/src/gallium/drivers/nouveau/codegen/nv50_ir_target_nvc0.cpp
> +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_target_nvc0.cpp
> @@ -191,17 +191,15 @@ void TargetNVC0::initOpInfo()
>  {
>     unsigned int i, j;
>
> -   static const uint32_t commutative[(OP_LAST + 31) / 32] =
> +   static const operation commutative[] =
>     {
> -      // ADD, MUL, MAD, FMA, AND, OR, XOR, MAX, MIN, SET_AND, SET_OR, SET_XOR,
> -      // SET, SELP, SLCT
> -      0x0ce0ca00, 0x0000007e, 0x00000000, 0x00000000
> +      OP_ADD, OP_MUL, OP_MAD, OP_FMA, OP_AND, OP_OR, OP_XOR, OP_MAX, OP_MIN,
> +      OP_SET_AND, OP_SET_OR, OP_SET_XOR, OP_SET, OP_SELP, OP_SLCT
>     };
>
> -   static const uint32_t shortForm[(OP_LAST + 31) / 32] =
> +   static const operation shortForm[] =
>     {
> -      // ADD, MUL, MAD, FMA, AND, OR, XOR, MAX, MIN
> -      0x0ce0ca00, 0x00000000, 0x00000000, 0x00000000
> +      OP_ADD, OP_MUL, OP_MAD, OP_FMA, OP_AND, OP_OR, OP_XOR, OP_MAX, OP_MIN
>     };
>
>     static const operation noDest[] =
> @@ -240,12 +238,16 @@ void TargetNVC0::initOpInfo()
>
>        opInfo[i].hasDest = 1;
>        opInfo[i].vector = (i >= OP_TEX && i <= OP_TEXCSAA);
> -      opInfo[i].commutative = (commutative[i / 32] >> (i % 32)) & 1;
> +      opInfo[i].commutative = false;
>        opInfo[i].pseudo = (i < OP_MOV);
>        opInfo[i].predicate = !opInfo[i].pseudo;
>        opInfo[i].flow = (i >= OP_BRA && i <= OP_JOIN);
> -      opInfo[i].minEncSize = (shortForm[i / 32] & (1 << (i % 32))) ? 4 : 8;
> +      opInfo[i].minEncSize = 8;
>     }
> +   for (i = 0; i < sizeof(commutative) / sizeof(commutative[0]); ++i)
> +      opInfo[commutative[i]].commutative = true;
> +   for (i = 0; i < sizeof(shortForm) / sizeof(shortForm[0]); ++i)
> +      opInfo[shortForm[i]].minEncSize = 4;

same comment here

>     for (i = 0; i < sizeof(noDest) / sizeof(noDest[0]); ++i)
>        opInfo[noDest[i]].hasDest = 0;
>     for (i = 0; i < sizeof(noPred) / sizeof(noPred[0]); ++i)
> --
> 2.14.4
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev

with the type fixed: Reviewed-by: Karol Herbst <kherbst at redhat.com>