[Mesa-dev] [PATCH 09/12] nir: add legal bit_sizes to intrinsics
Jason Ekstrand
jason at jlekstrand.net
Mon Jan 7 17:16:35 UTC 2019
On Tue, Dec 4, 2018 at 12:27 PM Karol Herbst <kherbst at redhat.com> wrote:
> With OpenCL some system values match the address bits, but in GLSL we also
> have some system values being 64 bit like subgroup masks.
>
> With this it is possible to adjust the builder functions so that depending
> on the bit_sizes the correct bit_size is used or an additional argument is
> added in case of multiple possible values.
>
> v2: validate dest bit_size
>
> Signed-off-by: Karol Herbst <kherbst at redhat.com>
> ---
> src/compiler/nir/nir.h | 3 +++
> src/compiler/nir/nir_intrinsics.py | 25 +++++++++++++++----------
> src/compiler/nir/nir_intrinsics_c.py | 6 +++++-
> src/compiler/nir/nir_validate.c | 6 ++++++
> 4 files changed, 29 insertions(+), 11 deletions(-)
>
> diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h
> index e9f8f15d387..c5ea8dcdd1e 100644
> --- a/src/compiler/nir/nir.h
> +++ b/src/compiler/nir/nir.h
> @@ -1297,6 +1297,9 @@ typedef struct {
>
> /** semantic flags for calls to this intrinsic */
> nir_intrinsic_semantic_flag flags;
> +
> + /** bitfield of legal bit sizes */
> + unsigned bit_sizes : 7;
>
This should be called dest_bit_sizes and be after dest_components. Also
the bitfield :7 is really pointless given how many other things we have in
this struct that are simply declared "unsigned". If we're going to make it
a bitfield (probably a good idea anyway), we should do so across the board.
> } nir_intrinsic_info;
>
> extern const nir_intrinsic_info nir_intrinsic_infos[nir_num_intrinsics];
> diff --git a/src/compiler/nir/nir_intrinsics.py
> b/src/compiler/nir/nir_intrinsics.py
> index 6ea6ad1198f..830c406b450 100644
> --- a/src/compiler/nir/nir_intrinsics.py
> +++ b/src/compiler/nir/nir_intrinsics.py
> @@ -32,7 +32,7 @@ class Intrinsic(object):
> NOTE: this must be kept in sync with nir_intrinsic_info.
> """
> def __init__(self, name, src_components, dest_components,
> - indices, flags, sysval):
> + indices, flags, sysval, bit_sizes):
> """Parameters:
>
> - name: the intrinsic name
> @@ -45,6 +45,7 @@ class Intrinsic(object):
> - indices: list of constant indicies
> - flags: list of semantic flags
> - sysval: is this a system-value intrinsic
> + - bit_sizes: allowed dest bit_sizes
> """
> assert isinstance(name, str)
> assert isinstance(src_components, list)
> @@ -58,6 +59,8 @@ class Intrinsic(object):
> if flags:
> assert isinstance(flags[0], str)
> assert isinstance(sysval, bool)
> + if bit_sizes:
> + assert isinstance(bit_sizes[0], int)
>
> self.name = name
> self.num_srcs = len(src_components)
> @@ -68,6 +71,7 @@ class Intrinsic(object):
> self.indices = indices
> self.flags = flags
> self.sysval = sysval
> + self.bit_sizes = bit_sizes
>
> #
> # Possible indices:
> @@ -123,10 +127,10 @@ CAN_REORDER = "NIR_INTRINSIC_CAN_REORDER"
> INTR_OPCODES = {}
>
> def intrinsic(name, src_comp=[], dest_comp=-1, indices=[],
> - flags=[], sysval=False):
> + flags=[], sysval=False, bit_sizes=[]):
> assert name not in INTR_OPCODES
> INTR_OPCODES[name] = Intrinsic(name, src_comp, dest_comp,
> - indices, flags, sysval)
> + indices, flags, sysval, bit_sizes)
>
> intrinsic("nop", flags=[CAN_ELIMINATE])
>
> @@ -448,9 +452,10 @@ intrinsic("shared_atomic_fmin", src_comp=[1, 1],
> dest_comp=1, indices=[BASE])
> intrinsic("shared_atomic_fmax", src_comp=[1, 1], dest_comp=1,
> indices=[BASE])
> intrinsic("shared_atomic_fcomp_swap", src_comp=[1, 1, 1], dest_comp=1,
> indices=[BASE])
>
> -def system_value(name, dest_comp, indices=[]):
> +def system_value(name, dest_comp, indices=[], bit_sizes=[32]):
> intrinsic("load_" + name, [], dest_comp, indices,
> - flags=[CAN_ELIMINATE, CAN_REORDER], sysval=True)
> + flags=[CAN_ELIMINATE, CAN_REORDER], sysval=True,
> + bit_sizes=bit_sizes)
>
> system_value("frag_coord", 4)
> system_value("front_face", 1)
> @@ -485,11 +490,11 @@ system_value("layer_id", 1)
> system_value("view_index", 1)
> system_value("subgroup_size", 1)
> system_value("subgroup_invocation", 1)
> -system_value("subgroup_eq_mask", 0)
> -system_value("subgroup_ge_mask", 0)
> -system_value("subgroup_gt_mask", 0)
> -system_value("subgroup_le_mask", 0)
> -system_value("subgroup_lt_mask", 0)
> +system_value("subgroup_eq_mask", 0, bit_sizes=[32, 64])
> +system_value("subgroup_ge_mask", 0, bit_sizes=[32, 64])
> +system_value("subgroup_gt_mask", 0, bit_sizes=[32, 64])
> +system_value("subgroup_le_mask", 0, bit_sizes=[32, 64])
> +system_value("subgroup_lt_mask", 0, bit_sizes=[32, 64])
> system_value("num_subgroups", 1)
> system_value("subgroup_id", 1)
> system_value("local_group_size", 3)
> diff --git a/src/compiler/nir/nir_intrinsics_c.py
> b/src/compiler/nir/nir_intrinsics_c.py
> index ac45b94d496..d0f1c29fa39 100644
> --- a/src/compiler/nir/nir_intrinsics_c.py
> +++ b/src/compiler/nir/nir_intrinsics_c.py
> @@ -1,3 +1,5 @@
> +from functools import reduce
> +import operator
>
> template = """\
> /* Copyright (C) 2018 Red Hat
> @@ -45,6 +47,7 @@ const nir_intrinsic_info
> nir_intrinsic_infos[nir_num_intrinsics] = {
> },
> % endif
> .flags = ${"0" if len(opcode.flags) == 0 else " |
> ".join(opcode.flags)},
> + .bit_sizes = ${reduce(operator.or_, opcode.bit_sizes, 0)},
>
Mind doing "0x${hex(reduce(...))}" to make the C more readable?
> },
> % endfor
> };
> @@ -54,6 +57,7 @@ from nir_intrinsics import INTR_OPCODES
> from mako.template import Template
> import argparse
> import os
> +import functools
>
I don't see this being used anywhere since you import the two things you
need from it above.
>
> def main():
> parser = argparse.ArgumentParser()
> @@ -64,7 +68,7 @@ def main():
>
> path = os.path.join(args.outdir, 'nir_intrinsics.c')
> with open(path, 'wb') as f:
> - f.write(Template(template,
> output_encoding='utf-8').render(INTR_OPCODES=INTR_OPCODES))
> + f.write(Template(template,
> output_encoding='utf-8').render(INTR_OPCODES=INTR_OPCODES, reduce=reduce,
> operator=operator))
>
> if __name__ == '__main__':
> main()
> diff --git a/src/compiler/nir/nir_validate.c
> b/src/compiler/nir/nir_validate.c
> index ef24e96ee3f..428cf5671c3 100644
> --- a/src/compiler/nir/nir_validate.c
> +++ b/src/compiler/nir/nir_validate.c
> @@ -544,9 +544,15 @@ validate_intrinsic_instr(nir_intrinsic_instr *instr,
> validate_state *state)
>
> if (nir_intrinsic_infos[instr->intrinsic].has_dest) {
> unsigned components_written = nir_intrinsic_dest_components(instr);
> + unsigned bit_sizes =
> nir_intrinsic_infos[instr->intrinsic].bit_sizes;
>
> validate_assert(state, components_written > 0);
>
> + if (dest_bit_size && bit_sizes)
> + validate_assert(state, dest_bit_size & bit_sizes);
> + else
> + dest_bit_size = dest_bit_size ? dest_bit_size : bit_sizes;
>
I think this could be simpler. Maybe something such as
if (dest_bit_size)
validate_assert(state, nir_dest_bit_size(instr->dest) == dest_bit_size);
and then just pass bit_sizes through to validate_dest.
> +
> validate_dest(&instr->dest, state, dest_bit_size,
> components_written);
> }
> }
> --
> 2.19.2
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20190107/6de64f5b/attachment.html>
More information about the mesa-dev
mailing list