[Mesa-dev] [PATCH 09/12] nir: add legal bit_sizes to intrinsics
Karol Herbst
kherbst at redhat.com
Tue Jan 8 19:03:14 UTC 2019
On Mon, Jan 7, 2019 at 6:16 PM Jason Ekstrand <jason at jlekstrand.net> wrote:
>
> 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.
>
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
>>
More information about the mesa-dev
mailing list