<div dir="ltr"><div>In case anyone is wondering my opinion based on my experience writing both patch series, I think it's decidedly in the version B camp. The code changes required to nir_algebraic and nir_search are almost identical between the two besides the whole virtual opcode thing and the changes to front-end NIR producers are basically the same as well due to having to put 32 suffixes on some things. However, the changes to other parts of NIR aren't required and I feel they're very dirty and special-casey in version A.</div><div> </div><div>--Jason </div></div> <div class="gmail_quote"><div dir="ltr">On Thu, Nov 8, 2018 at 9:45 PM Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Welcome to this choose-your-own-adventure patch series! The first six patches are good-to-go regardless of choices made later. However, starting with patch 7, we have a choice to make. Version A of this series reworks NIR conversion opcodes to not have sizes and instead simply accept any source and destination size regardless of whether or not they match. This is something that Connor and I talked about some time ago when we were first adding bit sizes. With most opcodes, if they have any variable-size sources or a variable size destination, all variable sized things must match. This series changes the rules so that, for conversion ops, they have a variable size source and destination which DO NOT have to match. The upside to this is that we get to throw away piles of opcodes. The downside is that it makes a number of things: validation, nir_search, constant folding, etc. more complicated because they now have to deal with a new set of rules. Version B of the series, reworks boolean conversions to be like the current conversions and also have bit sizes. Unfortunately, this makes hash of opt_algebraic because, thanks to some applications love of re-inventing Boolean operations, we have piles of optimizations that contain b2f, f2b, b2i and i2b. To deal with that problem (and make opt_algebraic easier to use in general), version B adds virtual unsized conversion opcodes to nir_search so that you can make an expression that matches any i2f opcode regardless of size. From the perspective of opt_algebraic, then, the two series are nearly identical. The downside of this approach is that the nir_search changes are kind-of hacky and we still have piles of conversion opcodes. The upside, however, is that we don't have the extra mental overhead in constant folding, validation, and elsewhere of having this opcode special-case. What is the fate of NIR? That's up to the people to decide! Cc: Ian Romanick <<a href="mailto:ian.d.romanick@intel.com" target="_blank">ian.d.romanick@intel.com</a>> Cc: Connor Abbott <<a href="mailto:cwabbott0@gmail.com" target="_blank">cwabbott0@gmail.com</a>> Jason Ekstrand COMMON (6): nir/opcodes: Pull in the type helpers from constant_expressions nir/opcodes: Rename tbool to tbool32 nir/algebraic: Improve a couple error messages nir/algebraic: Set variable classes to the explicit bit size nir/algebraic: Clean up some __str__ cruft nir/algebraic: Improve error messages for replace expressions Jason Ekstrand VERSION A (9): nir: Add a concept of an unsized conversion opcode nir/builder: Create sized helpers for unsized conversion opcodes nir/constant_expressions: Handle unsized conversion ops nir/algebraic: Add support for unsized conversion opcodes nir: Switch conversions to unsized opcodes FIXUP: Fix NIR producers and consumers to use unsized conversions FIXUP: nir/opt_algebraic: Drop bit-size suffixes from conversions nir/algebraic: Add 32-bit specifiers to a bunch of booleans nir: Make boolean conversions unsized like other types Jason Ekstrand VERSION B (8): nir/algebraic: Refactor codegen a bit nir/algebraic: Add support for unsized conversion opcodes nir/opt_algebraic: Simplify an optimization using the new search ops nir/opt_algebraic: Drop bit-size suffixes from conversions nir/opt_algebraic: Add 32-bit specifiers to a bunch of booleans nir: Make boolean conversions sized just like the others FIXUP: nir/opt_algebraic: Add suffixes to some x2b opcodes FIXUP: Fix NIR producers and consumers to use unsized conversions src/amd/common/ac_nir_to_llvm.c | 32 +-- src/compiler/glsl/glsl_to_nir.cpp | 2 +- src/compiler/nir/nir.c | 67 ++++++ src/compiler/nir/nir.h | 7 + src/compiler/nir/nir_algebraic.py | 174 +++++++++++---- src/compiler/nir/nir_builder.h | 30 ++- src/compiler/nir/nir_builder_opcodes_h.py | 16 +- src/compiler/nir/nir_constant_expressions.h | 3 +- src/compiler/nir/nir_constant_expressions.py | 59 +++--- src/compiler/nir/nir_loop_analyze.c | 7 +- src/compiler/nir/nir_lower_idiv.c | 2 +- src/compiler/nir/nir_lower_int64.c | 2 +- src/compiler/nir/nir_opcodes.py | 152 +++++++------ src/compiler/nir/nir_opcodes_c.py | 89 +------- src/compiler/nir/nir_opt_algebraic.py | 199 +++++++++--------- src/compiler/nir/nir_opt_constant_folding.c | 5 +- src/compiler/nir/nir_search.c | 19 +- src/compiler/nir/nir_validate.c | 4 + src/compiler/spirv/spirv_to_nir.c | 3 +- src/compiler/spirv/vtn_glsl450.c | 4 +- src/gallium/auxiliary/nir/tgsi_to_nir.c | 8 +- .../drivers/freedreno/ir3/ir3_compiler_nir.c | 148 ++++++------- src/gallium/drivers/vc4/vc4_program.c | 8 +- src/intel/compiler/brw_fs_nir.cpp | 78 +++---- src/intel/compiler/brw_vec4_nir.cpp | 39 ++-- src/mesa/program/prog_to_nir.c | 4 +- 26 files changed, 626 insertions(+), 535 deletions(-) -- 2.19.1 </blockquote></div>