[Mesa-dev] [PATCH v2 2/7] nir/spirv: propagate constants of GroupNonUniformQuad instructions, eliminate warning and fix breaks

[Jason Ekstrand]

On Fri, Mar 16, 2018 at 2:50 AM, Daniel Schürmann <
daniel.schuermann at campus.tu-berlin.de> wrote:

> Signed-off-by: Daniel Schürmann <daniel.schuermann at campus.tu-berlin.de>
> ---
>  src/compiler/spirv/spirv_to_nir.c | 2 ++
>  src/compiler/spirv/vtn_subgroup.c | 8 ++++++--
>  2 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/src/compiler/spirv/spirv_to_nir.c
> b/src/compiler/spirv/spirv_to_nir.c
> index f06dca90ef..4454c1aca3 100644
> --- a/src/compiler/spirv/spirv_to_nir.c
> +++ b/src/compiler/spirv/spirv_to_nir.c
> @@ -3340,10 +3340,12 @@ vtn_handle_preamble_instruction(struct
> vtn_builder *b, SpvOp opcode,
>
>        case SpvCapabilityGroupNonUniformQuad:
>           spv_check_supported(subgroup_quad, cap);
> +         break;
>
>        case SpvCapabilityGroupNonUniformArithmetic:
>        case SpvCapabilityGroupNonUniformClustered:
>           spv_check_supported(subgroup_arithmetic, cap);
> +         break;
>
>        case SpvCapabilityVariablePointersStorageBuffer:
>        case SpvCapabilityVariablePointers:
> diff --git a/src/compiler/spirv/vtn_subgroup.c b/src/compiler/spirv/vtn_
> subgroup.c
> index bd3143962b..73420b7e43 100644
> --- a/src/compiler/spirv/vtn_subgroup.c
> +++ b/src/compiler/spirv/vtn_subgroup.c
> @@ -261,7 +261,9 @@ vtn_handle_subgroup(struct vtn_builder *b, SpvOp
> opcode,
>     case SpvOpGroupNonUniformQuadBroadcast:
>        vtn_build_subgroup_instr(b, nir_intrinsic_quad_broadcast,
>                                 val->ssa, vtn_ssa_value(b, w[4]),
> -                               vtn_ssa_value(b, w[5])->def, 0, 0);
> +                               vtn_ssa_value(b, w[5])->def,
> +                               vtn_constant_value(b,
> w[5])->values[0].u32[0],
>

quad_broadcast has no constant value index.  See the comment below about
NIR not respecting it for copies etc.  If you want the constant value in
your back-end, nir_src_as_const_value will get that for you rather handily.


> +                               0);
>        break;
>
>     case SpvOpGroupNonUniformQuadSwap: {
> @@ -277,9 +279,11 @@ vtn_handle_subgroup(struct vtn_builder *b, SpvOp
> opcode,
>        case 2:
>           op = nir_intrinsic_quad_swap_diagonal;
>           break;
> +      default:
> +         vtn_fail("Invalid constant value in OpGroupNonUniformQuadSwap");
>

The breaks and this vtn_fail are all ok.


>        }
>        vtn_build_subgroup_instr(b, op, val->ssa, vtn_ssa_value(b, w[4]),
> -                               NULL, 0, 0);
> +                               NULL, direction, 0);
>

The quad operations, as defined in NIR, have no "direction" index.  Sure,
you can set the field in the data structure, but you're liable to have it
go missing if you clone or [de]serialize the IR and not be respected for
things such as CSE.  If your back-end depneds on this being set, you're
most likely going to run into weird bugs.  Use the opcode instead.


>        break;
>     }
>
> --
> 2.14.1
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180316/eccf21bd/attachment-0001.html>