Glenn, Thanks for taking the time to look over things. On Fri, Dec 19, 2014 at 1:24 PM, Glenn Kennard <glenn.kenn...@gmail.com> wrote: > > On Tue, 16 Dec 2014 07:04:14 +0100, Jason Ekstrand <ja...@jlekstrand.net> > wrote: > > From: Connor Abbott <connor.abb...@intel.com> >> >> This includes all the instructions, ifs, loops, functions, etc. This is >> similar to the information in ir.h. >> >> v2: Jason Ekstrand <jason.ekstr...@intel.com>: >> Include ralloc and hash_table from the util directory >> --- >> src/glsl/Makefile.sources | 2 + >> src/glsl/nir/nir.h | 1150 ++++++++++++++++++++++++++++++ >> +++++++++++ >> src/glsl/nir/nir_intrinsics.c | 49 ++ >> src/glsl/nir/nir_intrinsics.h | 158 ++++++ >> src/glsl/nir/nir_opcodes.c | 46 ++ >> src/glsl/nir/nir_opcodes.h | 346 +++++++++++++ >> 6 files changed, 1751 insertions(+) >> create mode 100644 src/glsl/nir/nir.h >> create mode 100644 src/glsl/nir/nir_intrinsics.c >> create mode 100644 src/glsl/nir/nir_intrinsics.h >> create mode 100644 src/glsl/nir/nir_opcodes.c >> create mode 100644 src/glsl/nir/nir_opcodes.h >> >> diff --git a/src/glsl/Makefile.sources b/src/glsl/Makefile.sources >> index c3a90f7..e8eedd1 100644 >> --- a/src/glsl/Makefile.sources >> +++ b/src/glsl/Makefile.sources >> @@ -14,6 +14,8 @@ LIBGLCPP_GENERATED_FILES = \ >> $(GLSL_BUILDDIR)/glcpp/glcpp-parse.c >> NIR_FILES = \ >> + $(GLSL_SRCDIR)/nir/nir_intrinsics.c \ >> + $(GLSL_SRCDIR)/nir/nir_opcodes.c \ >> $(GLSL_SRCDIR)/nir/nir_types.cpp >> # libglsl >> diff --git a/src/glsl/nir/nir.h b/src/glsl/nir/nir.h >> new file mode 100644 >> index 0000000..ef486da >> --- /dev/null >> +++ b/src/glsl/nir/nir.h >> @@ -0,0 +1,1150 @@ >> +/* >> + * Copyright © 2014 Connor Abbott >> + * >> + * Permission is hereby granted, free of charge, to any person obtaining >> a >> + * copy of this software and associated documentation files (the >> "Software"), >> + * to deal in the Software without restriction, including without >> limitation >> + * the rights to use, copy, modify, merge, publish, distribute, >> sublicense, >> + * and/or sell copies of the Software, and to permit persons to whom the >> + * Software is furnished to do so, subject to the following conditions: >> + * >> + * The above copyright notice and this permission notice (including the >> next >> + * paragraph) shall be included in all copies or substantial portions of >> the >> + * Software. >> + * >> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, >> EXPRESS OR >> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF >> MERCHANTABILITY, >> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT >> SHALL >> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR >> OTHER >> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, >> ARISING >> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER >> DEALINGS >> + * IN THE SOFTWARE. >> + * >> + * Authors: >> + * Connor Abbott (cwabbo...@gmail.com) >> + * >> + */ >> + >> +#pragma once >> + >> +#include "util/hash_table.h" >> +#include "main/set.h" >> +#include "../list.h" >> +#include "GL/gl.h" /* GLenum */ >> +#include "util/ralloc.h" >> +#include "nir_types.h" >> +#include <stdio.h> >> + >> +#ifdef __cplusplus >> +extern "C" { >> +#endif >> + >> +struct nir_function_overload; >> +struct nir_function; >> + >> + >> +/** >> + * Description of built-in state associated with a uniform >> + * >> + * \sa nir_variable::state_slots >> + */ >> +typedef struct { >> + int tokens[5]; >> + int swizzle; >> +} nir_state_slot; >> + >> +typedef enum { >> + nir_var_shader_in, >> + nir_var_shader_out, >> + nir_var_global, >> + nir_var_local, >> + nir_var_uniform, >> + nir_var_system_value >> +} nir_variable_mode; >> + >> +/** >> + * Data stored in an nir_constant >> + */ >> +union nir_constant_data { >> + unsigned u[16]; >> + int i[16]; >> + float f[16]; >> + bool b[16]; >> +}; >> + >> +typedef struct nir_constant { >> + /** >> + * Value of the constant. >> + * >> + * The field used to back the values supplied by the constant is >> determined >> + * by the type associated with the \c ir_instruction. Constants may >> be >> + * scalars, vectors, or matrices. >> + */ >> + union nir_constant_data value; >> + >> + /* Array elements / Structure Fields */ >> + struct nir_constant **elements; >> +} nir_constant; >> + >> +/** >> + * \brief Layout qualifiers for gl_FragDepth. >> + * >> + * The AMD/ARB_conservative_depth extensions allow gl_FragDepth to be >> redeclared >> + * with a layout qualifier. >> + */ >> +typedef enum { >> + nir_depth_layout_none, /**< No depth layout is specified. */ >> + nir_depth_layout_any, >> + nir_depth_layout_greater, >> + nir_depth_layout_less, >> + nir_depth_layout_unchanged >> +} nir_depth_layout; >> + >> +/** >> + * Either a uniform, global variable, shader input, or shader output. >> Based on >> + * ir_variable - it should be easy to translate between the two. >> + */ >> + >> +typedef struct { >> + struct exec_node node; >> + >> + /** >> + * Declared type of the variable >> + */ >> + const struct glsl_type *type; >> + >> + /** >> + * Declared name of the variable >> + */ >> + char *name; >> + >> + /** >> + * For variables which satisfy the is_interface_instance() predicate, >> this >> + * points to an array of integers such that if the ith member of the >> + * interface block is an array, max_ifc_array_access[i] is the maximum >> + * array element of that member that has been accessed. If the ith >> member >> + * of the interface block is not an array, max_ifc_array_access[i] is >> + * unused. >> + * >> + * For variables whose type is not an interface block, this pointer is >> + * NULL. >> + */ >> + unsigned *max_ifc_array_access; >> + >> + struct nir_variable_data { >> + >> + /** >> + * Is the variable read-only? >> + * >> + * This is set for variables declared as \c const, shader inputs, >> + * and uniforms. >> + */ >> + unsigned read_only:1; >> + unsigned centroid:1; >> + unsigned sample:1; >> + unsigned invariant:1; >> + >> + /** >> + * Storage class of the variable. >> + * >> + * \sa nir_variable_mode >> + */ >> + unsigned mode:4; >> + >> + /** >> + * Interpolation mode for shader inputs / outputs >> + * >> + * \sa ir_variable_interpolation >> + */ >> + unsigned interpolation:2; >> + >> + /** >> + * \name ARB_fragment_coord_conventions >> + * @{ >> + */ >> + unsigned origin_upper_left:1; >> + unsigned pixel_center_integer:1; >> + /*@}*/ >> + >> + /** >> + * Was the location explicitly set in the shader? >> + * >> + * If the location is explicitly set in the shader, it \b cannot >> be changed >> + * by the linker or by the API (e.g., calls to \c >> glBindAttribLocation have >> + * no effect). >> + */ >> + unsigned explicit_location:1; >> + unsigned explicit_index:1; >> + >> + /** >> + * Was an initial binding explicitly set in the shader? >> + * >> + * If so, constant_value contains an integer ir_constant >> representing the >> + * initial binding point. >> + */ >> + unsigned explicit_binding:1; >> + >> + /** >> + * Does this variable have an initializer? >> + * >> + * This is used by the linker to cross-validiate initializers of >> global >> + * variables. >> + */ >> + unsigned has_initializer:1; >> + >> + /** >> + * Is this variable a generic output or input that has not yet >> been matched >> + * up to a variable in another stage of the pipeline? >> + * >> + * This is used by the linker as scratch storage while assigning >> locations >> + * to generic inputs and outputs. >> + */ >> + unsigned is_unmatched_generic_inout:1; >> + >> + /** >> + * If non-zero, then this variable may be packed along with other >> variables >> + * into a single varying slot, so this offset should be applied >> when >> + * accessing components. For example, an offset of 1 means that >> the x >> + * component of this variable is actually stored in component y of >> the >> + * location specified by \c location. >> + */ >> + unsigned location_frac:2; >> + >> + /** >> + * Non-zero if this variable was created by lowering a named >> interface >> + * block which was not an array. >> + * >> + * Note that this variable and \c from_named_ifc_block_array will >> never >> + * both be non-zero. >> + */ >> + unsigned from_named_ifc_block_nonarray:1; >> + >> + /** >> + * Non-zero if this variable was created by lowering a named >> interface >> + * block which was an array. >> + * >> + * Note that this variable and \c from_named_ifc_block_nonarray >> will never >> + * both be non-zero. >> + */ >> + unsigned from_named_ifc_block_array:1; >> + >> + /** >> + * \brief Layout qualifier for gl_FragDepth. >> + * >> + * This is not equal to \c ir_depth_layout_none if and only if this >> + * variable is \c gl_FragDepth and a layout qualifier is specified. >> + */ >> + nir_depth_layout depth_layout; >> + >> + /** >> + * Storage location of the base of this variable >> + * >> + * The precise meaning of this field depends on the nature of the >> variable. >> + * >> + * - Vertex shader input: one of the values from \c >> gl_vert_attrib. >> + * - Vertex shader output: one of the values from \c >> gl_varying_slot. >> + * - Geometry shader input: one of the values from \c >> gl_varying_slot. >> + * - Geometry shader output: one of the values from \c >> gl_varying_slot. >> + * - Fragment shader input: one of the values from \c >> gl_varying_slot. >> + * - Fragment shader output: one of the values from \c >> gl_frag_result. >> + * - Uniforms: Per-stage uniform slot number for default uniform >> block. >> + * - Uniforms: Index within the uniform block definition for UBO >> members. >> + * - Other: This field is not currently used. >> + * >> + * If the variable is a uniform, shader input, or shader output, >> and the >> + * slot has not been assigned, the value will be -1. >> + */ >> + int location; >> + >> + /** >> + * The actual location of the variable in the IR. Only valid for >> inputs >> + * and outputs. >> + */ >> + unsigned int driver_location; >> + >> + /** >> + * output index for dual source blending. >> + */ >> + int index; >> > > src index is only ever 0 or 1. I'll note its already a bitfield in > src/glsl/ir.h from where this is copied, along with some other space > optimizations, might want to copy those over as well. > > + >> + /** >> + * Initial binding point for a sampler or UBO. >> + * >> + * For array types, this represents the binding point for the >> first element. >> + */ >> + int binding; >> + >> + /** >> + * Location an atomic counter is stored at. >> + */ >> + struct { >> + unsigned buffer_index; >> + unsigned offset; >> + } atomic; >> + >> + /** >> + * ARB_shader_image_load_store qualifiers. >> + */ >> + struct { >> + bool read_only; /**< "readonly" qualifier. */ >> + bool write_only; /**< "writeonly" qualifier. */ >> + bool coherent; >> + bool _volatile; >> + bool restrict_flag; >> + >> + /** Image internal format if specified explicitly, otherwise >> GL_NONE. */ >> + GLenum format; >> + } image; >> > > Similar to dual source index, image struct has diverged a bit from > src/glsl/ir.h. > > + >> + /** >> + * Highest element accessed with a constant expression array index >> + * >> + * Not used for non-array variables. >> + */ >> + unsigned max_array_access; >> + >> + } data; >> + >> + /** >> + * Built-in state that backs this uniform >> + * >> + * Once set at variable creation, \c state_slots must remain >> invariant. >> + * This is because, ideally, this array would be shared by all clones >> of >> + * this variable in the IR tree. In other words, we'd really like >> for it >> + * to be a fly-weight. >> + * >> + * If the variable is not a uniform, \c num_state_slots will be zero >> and >> + * \c state_slots will be \c NULL. >> + */ >> + /*@{*/ >> + unsigned num_state_slots; /**< Number of state slots used */ >> + nir_state_slot *state_slots; /**< State descriptors. */ >> + /*@}*/ >> + >> + /** >> + * Value assigned in the initializer of a variable declared "const" >> + */ >> + nir_constant *constant_value; >> + >> + /** >> + * Constant expression assigned in the initializer of the variable >> + * >> + * \warning >> + * This field and \c ::constant_value are distinct. Even if the two >> fields >> + * refer to constants with the same value, they must point to separate >> + * objects. >> + */ >> + nir_constant *constant_initializer; >> + >> + /** >> + * For variables that are in an interface block or are an instance of >> an >> + * interface block, this is the \c GLSL_TYPE_INTERFACE type for that >> block. >> + * >> + * \sa ir_variable::location >> + */ >> + const struct glsl_type *interface_type; >> +} nir_variable; >> + >> +typedef struct { >> + struct exec_node node; >> + >> + unsigned num_components; /** < number of vector components */ >> + unsigned num_array_elems; /** < size of array (0 for no array) */ >> + >> + /** for liveness analysis, the index in the bit-array of live >> variables */ >> + unsigned index; >> + >> + /** only for debug purposes, can be NULL */ >> + const char *name; >> + >> + /** whether this register is local (per-function) or global >> (per-shader) */ >> + bool is_global; >> + >> + /** >> + * If this flag is set to true, then accessing channels >= >> num_components >> + * is well-defined, and simply spills over to the next array element. >> This >> + * is useful for backends that can do per-component accessing, in >> + * particular scalar backends. By setting this flag and making >> + * num_components equal to 1, structures can be packed tightly into >> + * registers and then registers can be accessed per-component to get >> to >> + * each structure member, even if it crosses vec4 boundaries. >> + */ >> + bool is_packed; >> + >> + /** set of nir_instr's where this register is used (read from) */ >> + struct set *uses; >> + >> + /** set of nir_instr's where this register is defined (written to) */ >> + struct set *defs; >> + >> + /** set of ifs where this register is used as a condition */ >> + struct set *if_uses; >> +} nir_register; >> + >> +typedef enum { >> + nir_instr_type_alu, >> + nir_instr_type_call, >> + nir_instr_type_texture, >> + nir_instr_type_intrinsic, >> + nir_instr_type_load_const, >> + nir_instr_type_jump, >> + nir_instr_type_ssa_undef, >> + nir_instr_type_phi, >> +} nir_instr_type; >> + >> +typedef struct { >> + struct exec_node node; >> + nir_instr_type type; >> + struct nir_block *block; >> +} nir_instr; >> + >> +#define nir_instr_next(instr) \ >> + exec_node_data(nir_instr, (instr)->node.next, node) >> + >> +#define nir_instr_prev(instr) \ >> + exec_node_data(nir_instr, (instr)->node.prev, node) >> > > Static inlines similar what is done for nir_deref_as_var might make > walking the structures in gdb a little easier. > > + >> +typedef struct { >> + /** for debugging only, can be NULL */ >> + const char* name; >> + >> + /** index into the bit-array for liveness analysis */ >> + unsigned index; >> + >> + nir_instr *parent_instr; >> + >> + struct set *uses; >> + struct set *if_uses; >> > > Comment what type the sets are, similar to whats already done for > nir_register.uses. >
Sure, that's a good plan. > I think using sets for these is probably overkill, a simple array or > linked list is likely better performing for most cases. A number or two to > motivate the extra complexity perhaps? > Being able to quickly add/remove elements is extremely important. Just ask the noveau people about this. Why not a linked list? Part of the problem is that we have a many-to-many mapping. Multiple instructions can use the same source and a single instruction can use multiple sources. I did think about changing to a linked list with the link embedded in the source. However, this would require adding 3 pointers to every source (prev, next, and a pointer back to the instruction) in order be able to do it efficiently. Also, if you want to iterate over all of the instructions that use a source, you have to keep track of what instructions you've seen because the same instruction can use a source multiple times. In other words, It was deemed not practical. > > + >> + uint8_t num_components; >> +} nir_ssa_def; >> + >> +struct nir_src; >> + >> +typedef struct { >> + nir_register *reg; >> + struct nir_src *indirect; /** < NULL for no indirect offset */ >> + unsigned base_offset; >> + >> + /* TODO use-def chain goes here */ >> > > Stale comment? Same for nir_reg_dest > > > +} nir_reg_src; >> + >> +typedef struct { >> + nir_register *reg; >> + struct nir_src *indirect; /** < NULL for no indirect offset */ >> + unsigned base_offset; >> + >> + /* TODO def-use chain goes here */ >> +} nir_reg_dest; >> + >> +typedef struct nir_src { >> + union { >> + nir_reg_src reg; >> + nir_ssa_def *ssa; >> + }; >> + >> + bool is_ssa; >> +} nir_src; >> + >> +typedef struct { >> + union { >> + nir_reg_dest reg; >> + nir_ssa_def ssa; >> + }; >> + >> + bool is_ssa; >> +} nir_dest; >> + >> +nir_src nir_src_copy(nir_src src, void *mem_ctx); >> +nir_dest nir_dest_copy(nir_dest dest, void *mem_ctx); >> + >> +typedef struct { >> + nir_src src; >> + >> + /** >> + * \name input modifiers >> + */ >> + /*@{*/ >> + /** >> + * For inputs interpreted as a floating point, flips the sign bit. >> For inputs >> + * interpreted as an integer, performs the two's complement negation. >> + */ >> + bool negate; >> + >> + /** >> + * Clears the sign bit for floating point values, and computes the >> integer >> + * absolute value for integers. Note that the negate modifier acts >> after >> + * the absolute value modifier, therefore if both are set then all >> inputs >> + * will become negative. >> + */ >> + bool abs; >> + /*@}*/ >> + >> + /** >> + * For each input component, says which component of the register it >> is >> + * chosen from. Note that which elements of the swizzle are used and >> which >> + * are ignored are based on the write mask for most opcodes - for >> example, >> + * a statement like "foo.xzw = bar.zyx" would have a writemask of >> 1101b and >> + * a swizzle of {2, x, 1, 0} where x means "don't care." >> + */ >> + uint8_t swizzle[4]; >> +} nir_alu_src; >> + >> +typedef struct { >> + nir_dest dest; >> + >> + /** >> + * \name saturate output modifier >> + * >> + * Only valid for opcodes that output floating-point numbers. Clamps >> the >> + * output to between 0.0 and 1.0 inclusive. >> + */ >> + >> + bool saturate; >> + >> + unsigned write_mask : 4; /* ignored if dest.is_ssa is true */ >> +} nir_alu_dest; >> + >> +#define OPCODE(name, num_inputs, per_component, output_size, >> output_type, \ >> + input_sizes, input_types) \ >> + nir_op_##name, >> + >> +#define LAST_OPCODE(name) nir_last_opcode = nir_op_##name, >> + >> +typedef enum { >> +#include "nir_opcodes.h" >> + nir_num_opcodes = nir_last_opcode + 1 >> +} nir_op; >> + >> +#undef OPCODE >> +#undef LAST_OPCODE >> + >> +typedef enum { >> + nir_type_float, >> + nir_type_int, >> + nir_type_unsigned, >> + nir_type_bool >> +} nir_alu_type; >> + >> +typedef struct { >> + const char *name; >> + >> + unsigned num_inputs; >> + >> + /** >> + * If true, the opcode acts in the standard, per-component manner; the >> + * operation is performed on each component (except the ones that are >> masked >> + * out) with the input being taken from the input swizzle for that >> component. >> + * >> + * If false, the size of the output and inputs are explicitly given; >> swizzle >> + * and writemask are still in effect, but if the output component is >> masked >> + * out, then the input component may still be in use. >> + * >> + * The size of some of the inputs may be given (i.e. non-zero) even >> though >> + * per_component is false; in that case, each component of the input >> acts >> + * per-component, while the rest of the inputs and the output are >> normal. >> + * For example, for conditional select the condition is per-component >> but >> + * everything else is normal. >> + */ >> + bool per_component; >> + >> + /** >> + * If per_component is false, the number of components in the output. >> + */ >> + unsigned output_size; >> + >> + /** >> + * The type of vector that the instruction outputs. Note that this >> + * determines whether the saturate modifier is allowed. >> + */ >> + >> + nir_alu_type output_type; >> + >> + /** >> + * If per_component is false, the number of components in each input. >> + */ >> + unsigned input_sizes[4]; >> + >> + /** >> + * The type of vector that each input takes. Note that negate is only >> + * allowed on inputs with int or float type, and behaves differently >> on the >> + * two, and absolute value is only allowed on float type inputs. >> + */ >> + nir_alu_type input_types[4]; >> +} nir_op_info; >> + >> +extern const nir_op_info nir_op_infos[nir_num_opcodes]; >> + >> +typedef struct nir_alu_instr { >> + nir_instr instr; >> + nir_op op; >> + bool has_predicate; >> + nir_src predicate; >> + nir_alu_dest dest; >> + nir_alu_src src[]; >> +} nir_alu_instr; >> + >> +/* is this source channel used? */ >> +static inline bool >> +nir_alu_instr_channel_used(nir_alu_instr *instr, unsigned src, unsigned >> channel) >> +{ >> + if (nir_op_infos[instr->op].input_sizes[src] > 0) >> + return channel < nir_op_infos[instr->op].input_sizes[src]; >> + >> + return (instr->dest.write_mask >> channel) & 1; >> > > If per_component is false this should probably return true if any input > channel is used? per_component is dead as of 136/133, we just use the input/output sizes so this is correct. > +} >> + >> +typedef enum { >> + nir_deref_type_var, >> + nir_deref_type_array, >> + nir_deref_type_struct >> +} nir_deref_type; >> + >> +typedef struct nir_deref { >> + nir_deref_type deref_type; >> + struct nir_deref *child; >> + const struct glsl_type *type; >> +} nir_deref; >> + >> +typedef struct { >> + nir_deref deref; >> + >> + nir_variable *var; >> +} nir_deref_var; >> + >> +typedef struct { >> + nir_deref deref; >> + >> + unsigned base_offset; >> + bool has_indirect; >> + nir_src indirect; >> +} nir_deref_array; >> + >> +typedef struct { >> + nir_deref deref; >> + >> + const char *elem; >> +} nir_deref_struct; >> + >> +#define nir_deref_as_var(_deref) exec_node_data(nir_deref_var, _deref, >> deref) >> +#define nir_deref_as_array(_deref) \ >> + exec_node_data(nir_deref_array, _deref, deref) >> +#define nir_deref_as_struct(_deref) \ >> + exec_node_data(nir_deref_struct, _deref, deref) >> + >> +typedef struct { >> + nir_instr instr; >> + >> + unsigned num_params; >> + nir_deref_var **params; >> + nir_deref_var *return_deref; >> + >> + bool has_predicate; >> + nir_src predicate; >> + >> + struct nir_function_overload *callee; >> +} nir_call_instr; >> + >> +#define INTRINSIC(name, num_srcs, src_components, has_dest, >> dest_components, \ >> + num_variables, num_indices, flags) \ >> + nir_intrinsic_##name, >> + >> +#define LAST_INTRINSIC(name) nir_last_intrinsic = nir_intrinsic_##name, >> + >> +typedef enum { >> +#include "nir_intrinsics.h" >> + nir_num_intrinsics = nir_last_intrinsic + 1 >> +} nir_intrinsic_op; >> + >> +#undef INTRINSIC >> +#undef LAST_INTRINSIC >> + >> +typedef struct { >> + nir_instr instr; >> + >> + nir_intrinsic_op intrinsic; >> + >> + nir_dest dest; >> + >> + int const_index[3]; >> + >> + nir_deref_var *variables[2]; >> + >> + bool has_predicate; >> + nir_src predicate; >> + >> + nir_src src[]; >> +} nir_intrinsic_instr; >> + >> +/** >> + * \name NIR intrinsics semantic flags >> + * >> + * information about what the compiler can do with the intrinsics. >> + * >> + * \sa nir_intrinsic_info::flags >> + */ >> +/*@{*/ >> +/** >> + * whether the intrinsic can be safely eliminated if none of its register >> + * outputs are being used. >> + */ >> +#define NIR_INTRINSIC_CAN_ELIMINATE (1 << 0) >> + >> > > Make these enums, easier for debugging. > > +/** >> + * Whether the intrinsic can be reordered with respect to any other >> intrinsic, >> + * i.e. whether the only reodering dependencies of the intrinsic are due >> to the >> > > reordering > > Might want to note that even though an instruction itself is reorderable, > it still may not cross another non-reorderable instruction such as a > tessellation barrier instruction. > > > + * register reads/writes. >> + */ >> +#define NIR_INTRINSIC_CAN_REORDER (1 << 1) >> +/*@}*/ >> + >> +#define NIR_INTRINSIC_MAX_INPUTS 4 >> + >> +typedef struct { >> + const char *name; >> + >> + unsigned num_srcs; /** < number of register/SSA inputs */ >> + >> + /** number of components of each input register */ >> + unsigned src_components[NIR_INTRINSIC_MAX_INPUTS]; >> + >> + bool has_dest; >> + >> + /** number of components of each output register */ >> + unsigned dest_components; >> + >> + /** the number of inputs/outputs that are variables */ >> + unsigned num_variables; >> + >> + /** the number of constant indices used by the intrinsic */ >> + unsigned num_indices; >> + >> + /** semantic flags for calls to this intrinsic */ >> + unsigned flags; >> +} nir_intrinsic_info; >> + >> +extern const nir_intrinsic_info nir_intrinsic_infos[nir_num_intrinsics]; >> + >> +/** >> + * \group texture information >> + * >> + * This gives semantic information about textures which is useful to the >> + * frontend, the backend, and lowering passes, but not the optimizer. >> + */ >> + >> +typedef enum { >> + nir_tex_src_coord, >> + nir_tex_src_projector, >> + nir_tex_src_comparitor, /* shadow comparitor */ >> > > comparator thanks > > > + nir_tex_src_offset, >> + nir_tex_src_bias, >> + nir_tex_src_lod, >> + nir_tex_src_ms_index, /* MSAA sample index */ >> + nir_tex_src_ddx, >> + nir_tex_src_ddy, >> + nir_tex_src_sampler_index, /* < dynamically uniform indirect index */ >> + nir_num_texinput_types >> +} nir_texinput_type; >> + >> +typedef enum { >> + nir_texop_tex, /**< Regular texture look-up */ >> + nir_texop_txb, /**< Texture look-up with LOD bias */ >> + nir_texop_txl, /**< Texture look-up with explicit LOD >> */ >> + nir_texop_txd, /**< Texture look-up with partial >> derivatvies */ >> + nir_texop_txf, /**< Texel fetch with explicit LOD */ >> + nir_texop_txf_ms, /**< Multisample texture fetch */ >> + nir_texop_txs, /**< Texture size */ >> + nir_texop_lod, /**< Texture lod query */ >> + nir_texop_tg4, /**< Texture gather */ >> + nir_texop_query_levels /**< Texture levels query */ >> +} nir_texop; >> + >> +typedef struct { >> + nir_instr instr; >> + >> + bool has_predicate; >> + nir_src predicate; >> + >> + enum glsl_sampler_dim sampler_dim; >> + nir_alu_type dest_type; >> + >> + nir_texop op; >> + nir_dest dest; >> + nir_src src[4]; >> + nir_texinput_type src_type[4]; >> + unsigned num_srcs, coord_components; >> > > num_coord_components for consistency? > > + bool is_array, is_shadow; >> + >> + /** >> + * If is_shadow is true, whether this is the old-style shadow that >> outputs 4 >> + * components or the new-style shadow that outputs 1 component. >> + */ >> + bool is_new_style_shadow; >> > > is_four_component_shadow would be clearer, "new" will get old over the > lifespan of this code... Point. > > + >> + /* constant offset - must be 0 if the offset source is used */ >> + int const_offset[4]; >> + >> + /* gather component selector */ >> + unsigned component : 2; >> + >> + unsigned sampler_index; >> + nir_deref_var *sampler; /* if this is NULL, use sampler_index instead >> */ >> +} nir_tex_instr; >> + >> +static inline unsigned >> +nir_tex_instr_dest_size(nir_tex_instr *instr) >> +{ >> + if (instr->op == nir_texop_txs) { >> + unsigned ret; >> + switch (instr->sampler_dim) { >> + case GLSL_SAMPLER_DIM_1D: >> + case GLSL_SAMPLER_DIM_BUF: >> + ret = 1; >> + break; >> + case GLSL_SAMPLER_DIM_2D: >> + case GLSL_SAMPLER_DIM_CUBE: >> + case GLSL_SAMPLER_DIM_MS: >> + case GLSL_SAMPLER_DIM_RECT: >> + case GLSL_SAMPLER_DIM_EXTERNAL: >> + ret = 2; >> + break; >> + case GLSL_SAMPLER_DIM_3D: >> + ret = 3; >> + break; >> + default: >> + assert(0); >> + break; >> + } >> + if (instr->is_array) >> + ret++; >> + return ret; >> + } >> + >> + if (instr->op == nir_texop_query_levels) >> + return 2; >> + >> + if (instr->is_shadow && instr->is_new_style_shadow) >> + return 1; >> + >> + return 4; >> +} >> + >> +static inline unsigned >> +nir_tex_instr_src_size(nir_tex_instr *instr, unsigned src) >> +{ >> + if (instr->src_type[src] == nir_tex_src_coord) >> + return instr->coord_components; >> + >> + >> + if (instr->src_type[src] == nir_tex_src_offset || >> + instr->src_type[src] == nir_tex_src_ddx || >> + instr->src_type[src] == nir_tex_src_ddy) { >> + if (instr->is_array) >> + return instr->coord_components - 1; >> + else >> + return instr->coord_components; >> > > Please avoid mixing 0 and 1 indexing in the same variable if at all > possible. > That's not really what's going on here. In GLSL, if you have an ND array texture, it takes (N+1)D coordinates. You could make the case that NIR should use an extra source for this, but there's precedent. > > + } >> + >> + return 1; >> +} >> + >> +static inline int >> +nir_tex_instr_src_index(nir_tex_instr *instr, nir_texinput_type type) >> +{ >> + for (unsigned i = 0; i < instr->num_srcs; i++) >> + if (instr->src_type[i] == type) >> + return (int) i; >> + >> + return -1; >> +} >> + >> +typedef struct { >> + union { >> + float f[4]; >> + int32_t i[4]; >> + uint32_t u[4]; >> + }; >> +} nir_const_value; >> + >> > > Matrix types have been lowered before these are constructed? Comparing > with nir_constant_data which can fit a mat4. Yes. > > > +typedef struct { >> + nir_instr instr; >> + >> + union { >> + nir_const_value value; >> + nir_const_value *array; >> + }; >> + >> + unsigned num_components; >> + >> + /** >> + * The number of constant array elements to be copied into the >> variable. If >> + * this != 0, then value.array holds the array of size array_elems; >> + * otherwise, value.value holds the single vector constant (the more >> common >> + * case, and the only case for SSA destinations). >> + */ >> + unsigned array_elems; >> + >> + bool has_predicate; >> + nir_src predicate; >> + >> + nir_dest dest; >> +} nir_load_const_instr; >> + >> +typedef enum { >> + nir_jump_return, >> + nir_jump_break, >> + nir_jump_continue, >> +} nir_jump_type; >> + >> +typedef struct { >> + nir_instr instr; >> + nir_jump_type type; >> +} nir_jump_instr; >> + >> +/* creates a new SSA variable in an undefined state */ >> + >> +typedef struct { >> + nir_instr instr; >> + nir_ssa_def def; >> +} nir_ssa_undef_instr; >> + >> +typedef struct { >> + struct exec_node node; >> + struct nir_block *pred; >> > > A comment that pred here = predecessor. > Sure > + nir_src src; >> +} nir_phi_src; >> + >> +typedef struct { >> + nir_instr instr; >> + >> + struct exec_list srcs; >> > > Comment on real type of srcs. Same for all the exec_list/set/hash_table > types in this file that don't already have such comments. > Probably a good idea. > > + nir_dest dest; >> +} nir_phi_instr; >> + >> +#define nir_instr_as_alu(_instr) exec_node_data(nir_alu_instr, _instr, >> instr) >> +#define nir_instr_as_call(_instr) exec_node_data(nir_call_instr, _instr, >> instr) >> +#define nir_instr_as_jump(_instr) exec_node_data(nir_jump_instr, _instr, >> instr) >> +#define nir_instr_as_texture(_instr) \ >> + exec_node_data(nir_tex_instr, _instr, instr) >> > > Note actual type name "tex" but function "texture". Slightly confusing. Fixed in a later patch > > > +#define nir_instr_as_intrinsic(_instr) \ >> + exec_node_data(nir_intrinsic_instr, _instr, instr) >> +#define nir_instr_as_load_const(_instr) \ >> + exec_node_data(nir_load_const_instr, _instr, instr) >> +#define nir_instr_as_ssa_undef(_instr) \ >> + exec_node_data(nir_ssa_undef_instr, _instr, instr) >> +#define nir_instr_as_phi(_instr) \ >> + exec_node_data(nir_phi_instr, _instr, instr) >> + >> + >> +/* >> + * Control flow >> + * >> + * Control flow consists of a tree of control flow nodes, which include >> + * if-statements and loops. The leaves of the tree are basic blocks, >> lists of >> + * instructions that always run start-to-finish. Each basic block also >> keeps >> + * track of its successors (blocks which may run immediately after the >> current >> + * block) and predecessors (blocks which could have run immediately >> before the >> + * current block). Each function also has a start block and an end block >> which >> + * all return statements point to (which is always empty). Together, all >> the >> + * blocks with their predecessors and successors make up the control flow >> + * graph (CFG) of the function. There are helpers that modify the tree of >> + * control flow nodes while modifying the CFG appropriately; these >> should be >> + * used instead of modifying the tree directly. >> + */ >> + >> +typedef enum { >> + nir_cf_node_block, >> + nir_cf_node_if, >> + nir_cf_node_loop, >> + nir_cf_node_function >> +} nir_cf_node_type; >> + >> +typedef struct nir_cf_node { >> + struct exec_node node; >> + nir_cf_node_type type; >> + struct nir_cf_node *parent; >> +} nir_cf_node; >> + >> +typedef struct nir_block { >> + nir_cf_node cf_node; >> + struct exec_list instr_list; >> + >> + unsigned index; >> > > Deserves a comment what this is > Sure > > + >> + /* >> + * Each block can only have up to 2 successors, so we put them in a >> simple >> + * array - no need for anything more complicated. >> + */ >> + struct nir_block *successors[2]; >> + >> + struct set *predecessors; >> > > Why a set and not a simple array/list? a) Ordering doesn't matter and that's explicit here. b) This gets modified on-the-fly every time the CFG gets modified so we want quick insertion/removal (not array) and c) a set really isn't that much more complex than a linked list given that we already have the datastructure. > > > +} nir_block; >> + >> +#define nir_block_first_instr(block) \ >> + exec_node_data(nir_instr, exec_list_get_head(&(block)->instr_list), >> node) >> +#define nir_block_last_instr(block) \ >> + exec_node_data(nir_instr, exec_list_get_tail(&(block)->instr_list), >> node) >> + >> +#define nir_foreach_instr(block, instr) \ >> + foreach_list_typed(nir_instr, instr, node, &(block)->instr_list) >> +#define nir_foreach_instr_reverse(block, instr) \ >> + foreach_list_typed_reverse(nir_instr, instr, node, >> &(block)->instr_list) >> +#define nir_foreach_instr_safe(block, instr) \ >> + foreach_list_typed_safe(nir_instr, instr, node, &(block)->instr_list) >> + >> +typedef struct { >> + nir_cf_node cf_node; >> + nir_src condition; >> + struct exec_list then_list; >> + struct exec_list else_list; >> +} nir_if; >> + >> +#define nir_if_first_then_node(if) \ >> + exec_node_data(nir_cf_node, exec_list_get_head(&(if)->then_list), >> node) >> +#define nir_if_last_then_node(if) \ >> + exec_node_data(nir_cf_node, exec_list_get_tail(&(if)->then_list), >> node) >> +#define nir_if_first_else_node(if) \ >> + exec_node_data(nir_cf_node, exec_list_get_head(&(if)->else_list), >> node) >> +#define nir_if_last_else_node(if) \ >> + exec_node_data(nir_cf_node, exec_list_get_tail(&(if)->else_list), >> node) >> + >> +typedef struct { >> + nir_cf_node cf_node; >> + struct exec_list body; >> +} nir_loop; >> + >> +#define nir_loop_first_cf_node(loop) \ >> + exec_node_data(nir_cf_node, exec_list_get_head(&(loop)->body), node) >> +#define nir_loop_last_cf_node(loop) \ >> + exec_node_data(nir_cf_node, exec_list_get_tail(&(loop)->body), node) >> + >> +typedef struct { >> + nir_cf_node cf_node; >> + >> + /** pointer to the overload of which this is an implementation */ >> + struct nir_function_overload *overload; >> + >> + struct exec_list body; /** < list of nir_cf_node */ >> + >> + nir_block *start_block, *end_block; >> + >> + /** list for all local variables in the function */ >> + struct exec_list locals; >> + >> + /** array of variables used as parameters */ >> + unsigned num_params; >> + nir_variable **params; >> + >> + /** variable used to hold the result of the function */ >> + nir_variable *return_var; >> + >> + /** list of local registers in the function */ >> + struct exec_list registers; >> + >> + /** next available local register index */ >> + unsigned reg_alloc; >> + >> + /** next available SSA value index */ >> + unsigned ssa_alloc; >> + >> + /* total number of basic blocks, only valid when block_index_dirty = >> false */ >> + unsigned num_blocks; >> + >> + bool block_index_dirty; >> +} nir_function_impl; >> + >> +#define nir_cf_node_next(_node) \ >> + exec_node_data(nir_cf_node, exec_node_get_next(&(_node)->node), node) >> + >> +#define nir_cf_node_prev(_node) \ >> + exec_node_data(nir_cf_node, exec_node_get_prev(&(_node)->node), node) >> + >> +#define nir_cf_node_is_first(_node) \ >> + exec_node_is_head_sentinel((_node)->node.prev) >> + >> +#define nir_cf_node_is_last(_node) \ >> + exec_node_is_tail_sentinel((_node)->node.next) >> + >> +#define nir_cf_node_as_block(node) \ >> + exec_node_data(nir_block, node, cf_node) >> + >> +#define nir_cf_node_as_if(node) \ >> + exec_node_data(nir_if, node, cf_node) >> + >> +#define nir_cf_node_as_loop(node) \ >> + exec_node_data(nir_loop, node, cf_node) >> + >> +#define nir_cf_node_as_function(node) \ >> + exec_node_data(nir_function_impl, node, cf_node) >> + >> +typedef enum { >> + nir_parameter_in, >> + nir_parameter_out, >> + nir_parameter_inout, >> +} nir_parameter_type; >> + >> +typedef struct { >> + nir_parameter_type param_type; >> + const struct glsl_type *type; >> +} nir_parameter; >> + >> +typedef struct nir_function_overload { >> + struct exec_node node; >> + >> + unsigned num_params; >> + nir_parameter *params; >> + const struct glsl_type *return_type; >> + >> + nir_function_impl *impl; /** < NULL if the overload is only declared >> yet */ >> + >> + /** pointer to the function of which this is an overload */ >> + struct nir_function *function; >> +} nir_function_overload; >> + >> +typedef struct nir_function { >> + struct exec_node node; >> + >> + struct exec_list overload_list; >> + const char *name; >> +} nir_function; >> + >> +#define nir_function_first_overload(func) \ >> + exec_node_data(nir_function_overload, \ >> + exec_list_get_head(&(func)->overload_list), node) >> + >> +typedef struct nir_shader { >> + /** hash table of name -> uniform */ >> + struct hash_table *uniforms; >> + >> + /** hash table of name -> input */ >> + struct hash_table *inputs; >> + >> + /** hash table of name -> output */ >> + struct hash_table *outputs; >> + >> + /** list of global variables in the shader */ >> + struct exec_list globals; >> + >> + struct exec_list system_values; >> + >> + struct exec_list functions; >> + >> + /** list of global registers in the shader */ >> + struct exec_list registers; >> + >> + /** structures used in this shader */ >> + unsigned num_user_structures; >> + struct glsl_type **user_structures; >> + >> + /** next available global register index */ >> + unsigned reg_alloc; >> +} nir_shader; >> + >> +#define nir_foreach_overload(shader, overload) \ >> + foreach_list_typed(nir_function, func, node, &(shader)->functions) \ >> + foreach_list_typed(nir_function_overload, overload, node, \ >> + &(func)->overload_list) >> + >> +#ifdef __cplusplus >> +} /* extern "C" */ >> +#endif >> > > ... snipped for brevity, no comments on remaining files of patch, LGTM > > > Above is really only nitpicks, ignore at will. Kudos for Connor/Jason for > NIR, very impressive! > > Note i have reviewed the dd1f3c71f2d5253a69469f55cfa53dc3e5793b80 version > at > http://cgit.freedesktop.org/~jekstrand/mesa/tree/src/glsl/ > nir/nir.h?h=review/nir-v1 > rather than the exact version sent out to ml. > > Patch 004 is > Reviewed-By glenn.kennard <glenn.kenn...@gmail.com> > Thanks. I think I've addressed most of your comments. I decided to leave some of it alone for now. We've got quite a few patches on the list at this point and the little fixups can wait. --Jason
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev