On Mon, Jan 7, 2019 at 6:16 PM Jason Ekstrand <ja...@jlekstrand.net> wrote: > > 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. >
the biggest issue here is, that I don't want to require dest_bit_sizes to be set to a non 0 value, so we kind of have to take that into account. >> >> + >> 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