<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Tue, Dec 4, 2018 at 12:27 PM Karol Herbst <<a href="mailto:kherbst@redhat.com">kherbst@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">With OpenCL some system values match the address bits, but in GLSL we also<br>
have some system values being 64 bit like subgroup masks.<br>
<br>
With this it is possible to adjust the builder functions so that depending<br>
on the bit_sizes the correct bit_size is used or an additional argument is<br>
added in case of multiple possible values.<br>
<br>
v2: validate dest bit_size<br>
<br>
Signed-off-by: Karol Herbst <<a href="mailto:kherbst@redhat.com" target="_blank">kherbst@redhat.com</a>><br>
---<br>
 src/compiler/nir/nir.h               |  3 +++<br>
 src/compiler/nir/nir_intrinsics.py   | 25 +++++++++++++++----------<br>
 src/compiler/nir/nir_intrinsics_c.py |  6 +++++-<br>
 src/compiler/nir/nir_validate.c      |  6 ++++++<br>
 4 files changed, 29 insertions(+), 11 deletions(-)<br>
<br>
diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h<br>
index e9f8f15d387..c5ea8dcdd1e 100644<br>
--- a/src/compiler/nir/nir.h<br>
+++ b/src/compiler/nir/nir.h<br>
@@ -1297,6 +1297,9 @@ typedef struct {<br>
<br>
    /** semantic flags for calls to this intrinsic */<br>
    nir_intrinsic_semantic_flag flags;<br>
+<br>
+   /** bitfield of legal bit sizes */<br>
+   unsigned bit_sizes : 7;<br></blockquote><div><br></div><div>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.<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
 } nir_intrinsic_info;<br>
<br>
 extern const nir_intrinsic_info nir_intrinsic_infos[nir_num_intrinsics];<br>
diff --git a/src/compiler/nir/nir_intrinsics.py b/src/compiler/nir/nir_intrinsics.py<br>
index 6ea6ad1198f..830c406b450 100644<br>
--- a/src/compiler/nir/nir_intrinsics.py<br>
+++ b/src/compiler/nir/nir_intrinsics.py<br>
@@ -32,7 +32,7 @@ class Intrinsic(object):<br>
    NOTE: this must be kept in sync with nir_intrinsic_info.<br>
    """<br>
    def __init__(self, name, src_components, dest_components,<br>
-                indices, flags, sysval):<br>
+                indices, flags, sysval, bit_sizes):<br>
        """Parameters:<br>
<br>
        - name: the intrinsic name<br>
@@ -45,6 +45,7 @@ class Intrinsic(object):<br>
        - indices: list of constant indicies<br>
        - flags: list of semantic flags<br>
        - sysval: is this a system-value intrinsic<br>
+       - bit_sizes: allowed dest bit_sizes<br>
        """<br>
        assert isinstance(name, str)<br>
        assert isinstance(src_components, list)<br>
@@ -58,6 +59,8 @@ class Intrinsic(object):<br>
        if flags:<br>
            assert isinstance(flags[0], str)<br>
        assert isinstance(sysval, bool)<br>
+       if bit_sizes:<br>
+           assert isinstance(bit_sizes[0], int)<br>
<br>
        <a href="http://self.name" rel="noreferrer" target="_blank">self.name</a> = name<br>
        self.num_srcs = len(src_components)<br>
@@ -68,6 +71,7 @@ class Intrinsic(object):<br>
        self.indices = indices<br>
        self.flags = flags<br>
        self.sysval = sysval<br>
+       self.bit_sizes = bit_sizes<br>
<br>
 #<br>
 # Possible indices:<br>
@@ -123,10 +127,10 @@ CAN_REORDER   = "NIR_INTRINSIC_CAN_REORDER"<br>
 INTR_OPCODES = {}<br>
<br>
 def intrinsic(name, src_comp=[], dest_comp=-1, indices=[],<br>
-              flags=[], sysval=False):<br>
+              flags=[], sysval=False, bit_sizes=[]):<br>
     assert name not in INTR_OPCODES<br>
     INTR_OPCODES[name] = Intrinsic(name, src_comp, dest_comp,<br>
-                                   indices, flags, sysval)<br>
+                                   indices, flags, sysval, bit_sizes)<br>
<br>
 intrinsic("nop", flags=[CAN_ELIMINATE])<br>
<br>
@@ -448,9 +452,10 @@ intrinsic("shared_atomic_fmin",  src_comp=[1, 1], dest_comp=1, indices=[BASE])<br>
 intrinsic("shared_atomic_fmax",  src_comp=[1, 1], dest_comp=1, indices=[BASE])<br>
 intrinsic("shared_atomic_fcomp_swap", src_comp=[1, 1, 1], dest_comp=1, indices=[BASE])<br>
<br>
-def system_value(name, dest_comp, indices=[]):<br>
+def system_value(name, dest_comp, indices=[], bit_sizes=[32]):<br>
     intrinsic("load_" + name, [], dest_comp, indices,<br>
-              flags=[CAN_ELIMINATE, CAN_REORDER], sysval=True)<br>
+              flags=[CAN_ELIMINATE, CAN_REORDER], sysval=True,<br>
+              bit_sizes=bit_sizes)<br>
<br>
 system_value("frag_coord", 4)<br>
 system_value("front_face", 1)<br>
@@ -485,11 +490,11 @@ system_value("layer_id", 1)<br>
 system_value("view_index", 1)<br>
 system_value("subgroup_size", 1)<br>
 system_value("subgroup_invocation", 1)<br>
-system_value("subgroup_eq_mask", 0)<br>
-system_value("subgroup_ge_mask", 0)<br>
-system_value("subgroup_gt_mask", 0)<br>
-system_value("subgroup_le_mask", 0)<br>
-system_value("subgroup_lt_mask", 0)<br>
+system_value("subgroup_eq_mask", 0, bit_sizes=[32, 64])<br>
+system_value("subgroup_ge_mask", 0, bit_sizes=[32, 64])<br>
+system_value("subgroup_gt_mask", 0, bit_sizes=[32, 64])<br>
+system_value("subgroup_le_mask", 0, bit_sizes=[32, 64])<br>
+system_value("subgroup_lt_mask", 0, bit_sizes=[32, 64])<br>
 system_value("num_subgroups", 1)<br>
 system_value("subgroup_id", 1)<br>
 system_value("local_group_size", 3)<br>
diff --git a/src/compiler/nir/nir_intrinsics_c.py b/src/compiler/nir/nir_intrinsics_c.py<br>
index ac45b94d496..d0f1c29fa39 100644<br>
--- a/src/compiler/nir/nir_intrinsics_c.py<br>
+++ b/src/compiler/nir/nir_intrinsics_c.py<br>
@@ -1,3 +1,5 @@<br>
+from functools import reduce<br>
+import operator<br>
<br>
 template = """\<br>
 /* Copyright (C) 2018 Red Hat<br>
@@ -45,6 +47,7 @@ const nir_intrinsic_info nir_intrinsic_infos[nir_num_intrinsics] = {<br>
     },<br>
 % endif<br>
    .flags = ${"0" if len(opcode.flags) == 0 else " | ".join(opcode.flags)},<br>
+   .bit_sizes = ${reduce(operator.or_, opcode.bit_sizes, 0)},<br></blockquote><div><br></div><div>Mind doing "0x${hex(reduce(...))}" to make the C more readable?<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
 },<br>
 % endfor<br>
 };<br>
@@ -54,6 +57,7 @@ from nir_intrinsics import INTR_OPCODES<br>
 from mako.template import Template<br>
 import argparse<br>
 import os<br>
+import functools<br></blockquote><div><br></div><div>I don't see this being used anywhere since you import the two things you need from it above.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
 def main():<br>
     parser = argparse.ArgumentParser()<br>
@@ -64,7 +68,7 @@ def main():<br>
<br>
     path = os.path.join(args.outdir, 'nir_intrinsics.c')<br>
     with open(path, 'wb') as f:<br>
-        f.write(Template(template, output_encoding='utf-8').render(INTR_OPCODES=INTR_OPCODES))<br>
+        f.write(Template(template, output_encoding='utf-8').render(INTR_OPCODES=INTR_OPCODES, reduce=reduce, operator=operator))<br>
<br>
 if __name__ == '__main__':<br>
     main()<br>
diff --git a/src/compiler/nir/nir_validate.c b/src/compiler/nir/nir_validate.c<br>
index ef24e96ee3f..428cf5671c3 100644<br>
--- a/src/compiler/nir/nir_validate.c<br>
+++ b/src/compiler/nir/nir_validate.c<br>
@@ -544,9 +544,15 @@ validate_intrinsic_instr(nir_intrinsic_instr *instr, validate_state *state)<br>
<br>
    if (nir_intrinsic_infos[instr->intrinsic].has_dest) {<br>
       unsigned components_written = nir_intrinsic_dest_components(instr);<br>
+      unsigned bit_sizes = nir_intrinsic_infos[instr->intrinsic].bit_sizes;<br>
<br>
       validate_assert(state, components_written > 0);<br>
<br>
+      if (dest_bit_size && bit_sizes)<br>
+         validate_assert(state, dest_bit_size & bit_sizes);<br>
+      else<br>
+         dest_bit_size = dest_bit_size ? dest_bit_size : bit_sizes;<br></blockquote><div><br></div><div>I think this could be simpler.  Maybe something such as</div><div><br></div><div>if (dest_bit_size)</div><div>   validate_assert(state, nir_dest_bit_size(instr->dest) == dest_bit_size);</div><div><br></div><div>and then just pass bit_sizes through to validate_dest.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+<br>
       validate_dest(&instr->dest, state, dest_bit_size, components_written);<br>
    }<br>
 }<br>
-- <br>
2.19.2<br>
<br>
</blockquote></div></div>