[Mesa-dev] [PATCH 00/22] Send ir_assignment::condition to the Upside Down

[Alejandro Piñeiro]

Took a look to the series, and although it is not the part of the code
base Im more used too, it looks good to me (except some nitpicks pointed
on specific patches).

Having said so, I don't have too much to say on the two bits you mention
about patches 6 and patches 18, so probably you want someone with more
experience taking a closer look to them.

In any case:
Reviewed-by: Alejandro Piñeiro <apinheiro at igalia.com>

On 21/09/17 16:34, Ian Romanick wrote:
> From: Ian Romanick <ian.d.romanick at intel.com>
>
> I have also pushed this series to
>
> https://cgit.freedesktop.org/~idr/mesa/log/?h=remove-ir_assignment-condition
>
> I think -201 lines speaks for itself, but...
>
> The condition field pre-dates the ir_triop_csel operation by a few
> years.  The idea was that any assignment could be made conditional so
> that platforms that didn't have flow control could replace an
> if-statement with a bunch of conditional assignments.  However, handling
> this condition is really annoying in most optimization passes, so most
> passes bail as soon as a condition is encountered.
>
> Much later in development, ir_triop_csel was added.  For the most part,
> a conditional assignment is equivalent to a conditional select where the
> "false" value is the destination variable.
>
> My main motivation for this series is that I didn't want to deal with
> ir_assignment::condition in the SPIR-V generator work.  That I was able
> to delete a pile of code is a happy bonus.
>
> There are two odd bits that may warrant a bit closer examination:
>
> - Patch 6 changes the code that is generated for non-constant vector
>   indexing, and that results in some shader-db changes.  Most of the
>   changes are small (+1 instruction in a program), but some are more
>   substantial.
>
> - Patch 18 moves the 'foo = foo;' optimization from one optimization
>   pass to another.  If we just eliminate that optimization altogether,
>   shader-db results, even on platforms that use NIR, are hurt quite
>   substantially.  I have not investigated why NIR isn't picking up the
>   slack here.
>
> Ian Romanick (22):
>   glsl: Fix coding standards issues in lower_if_to_cond_assign
>   glsl: Lower ifs to conditional-select instead of conditional-assign
>   glsl: Fix coding standards issues in lower_vec_index_to_cond_assign
>   glsl: Return ir_variable from compare_index_block
>   glsl: Convert lower_vec_index_to_cond_assign to using ir_builder
>   glsl: Lower vector indexing to conditional-select instead of
>     conditional-assign
>   glsl: Fix coding standards issues in
>     lower_variable_index_to_cond_assign
>   glsl: Convert lower_variable_index_to_cond_assign to ir_builder
>   glsl: Lower array indexing to conditional-select instead of
>     conditional-assign
>   glsl: Don't pass NULL to ir_assignment constructor when not necessary
>   glsl/ast: Use ir_binop_equal instead of ir_binop_all_equal
>   glsl/ast: Convert ast_case_label::hir to ir_builder
>   glsl/ast: Explicitly track the set of case labels that occur after
>     default
>   glsl/ast: Generate a more compact expression to disable execution of
>     default case
>   glsl/ast: Use logical-or instead of conditional assignment to set
>     fallthru_var
>   glsl: Eliminate ir_builder assign overloads that have a condition
>   glsl: Eliminate ir_assignment constructors that have a condition
>   glsl: Move 'foo = foo;' optimization to opt_dead_code_local
>   glsl: Kill ir_assignment::condition with fire
>   glsl: Fix indentation left weird from the previous commit
>   glsl: Remove spurious assertions
>   glsl: Simplify ir_assignment::clone
>
>  src/compiler/glsl/ast_function.cpp            |  37 ++-
>  src/compiler/glsl/ast_to_hir.cpp              | 152 ++++++-----
>  src/compiler/glsl/glsl_to_nir.cpp             |  16 +-
>  src/compiler/glsl/ir.cpp                      |  18 +-
>  src/compiler/glsl/ir.h                        |  11 +-
>  src/compiler/glsl/ir_builder.cpp              |  15 +-
>  src/compiler/glsl/ir_builder.h                |   2 -
>  .../glsl/ir_builder_print_visitor.cpp         |   4 -
>  src/compiler/glsl/ir_clone.cpp                |  14 +-
>  src/compiler/glsl/ir_constant_expression.cpp  |  12 +-
>  .../glsl/ir_expression_flattening.cpp         |   4 +-
>  src/compiler/glsl/ir_hv_accept.cpp            |   3 -
>  src/compiler/glsl/ir_optimization.h           |   8 +-
>  src/compiler/glsl/ir_print_visitor.cpp        |   3 -
>  src/compiler/glsl/ir_reader.cpp               |   3 +-
>  src/compiler/glsl/ir_rvalue_visitor.cpp       |   2 -
>  src/compiler/glsl/loop_analysis.cpp           |   3 +-
>  src/compiler/glsl/loop_controls.cpp           |   2 +-
>  src/compiler/glsl/lower_discard.cpp           |   4 +-
>  src/compiler/glsl/lower_distance.cpp          |   2 +-
>  src/compiler/glsl/lower_if_to_cond_assign.cpp |  91 +++----
>  src/compiler/glsl/lower_instructions.cpp      |   4 +-
>  src/compiler/glsl/lower_jumps.cpp             |  11 +-
>  src/compiler/glsl/lower_mat_op_to_vec.cpp     |   2 +-
>  .../glsl/lower_texture_projection.cpp         |   2 +-
>  .../lower_variable_index_to_cond_assign.cpp   | 253 +++++++-----------
>  .../glsl/lower_vec_index_to_cond_assign.cpp   | 105 ++++----
>  src/compiler/glsl/lower_vector.cpp            |   4 +-
>  src/compiler/glsl/opt_array_splitting.cpp     |  10 +-
>  src/compiler/glsl/opt_constant_folding.cpp    |  17 --
>  .../glsl/opt_constant_propagation.cpp         |   3 -
>  src/compiler/glsl/opt_constant_variable.cpp   |   3 -
>  src/compiler/glsl/opt_copy_propagation.cpp    |  24 +-
>  .../glsl/opt_copy_propagation_elements.cpp    |   3 -
>  .../glsl/opt_dead_builtin_varyings.cpp        |   1 -
>  src/compiler/glsl/opt_dead_code_local.cpp     | 180 +++++++------
>  src/compiler/glsl/opt_function_inlining.cpp   |  11 +-
>  src/compiler/glsl/opt_structure_splitting.cpp |  11 +-
>  src/compiler/glsl/opt_tree_grafting.cpp       |   3 +-
>  src/compiler/glsl/opt_vectorize.cpp           |   3 +-
>  src/mesa/program/ir_to_mesa.cpp               |  24 +-
>  src/mesa/state_tracker/st_glsl_to_tgsi.cpp    |  17 +-
>  42 files changed, 448 insertions(+), 649 deletions(-)
>