On Tue, Dec 4, 2018 at 12:27 PM Karol Herbst <kher...@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 <kher...@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 > >
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev