On 05/16/2017 05:35 AM, Thomas Helland wrote: > 2017-05-16 3:31 GMT+02:00 Matt Turner <matts...@gmail.com>: >> On Mon, Apr 24, 2017 at 4:50 PM, Matt Turner <matts...@gmail.com> wrote: >>> On Thu, Apr 6, 2017 at 12:49 PM, Thomas Helland >>> <thomashellan...@gmail.com> wrote: >>>> The conditional discard pass follows the same pattern, so merge the >>>> two, and avoid running the visitor two times. >>>> --- >>>> src/compiler/Makefile.sources | 1 - >>>> src/compiler/glsl/glsl_parser_extras.cpp | 1 - >>>> src/compiler/glsl/ir_optimization.h | 1 - >>>> src/compiler/glsl/opt_conditional_discard.cpp | 88 >>>> --------------------------- >>>> src/compiler/glsl/opt_if_simplification.cpp | 50 +++++++++++---- >>>> 5 files changed, 39 insertions(+), 102 deletions(-) >>>> delete mode 100644 src/compiler/glsl/opt_conditional_discard.cpp >>>> >>>> diff --git a/src/compiler/Makefile.sources b/src/compiler/Makefile.sources >>>> index 5197c31bb3..817ab3a755 100644 >>>> --- a/src/compiler/Makefile.sources >>>> +++ b/src/compiler/Makefile.sources >>>> @@ -114,7 +114,6 @@ LIBGLSL_FILES = \ >>>> glsl/lower_ubo_reference.cpp \ >>>> glsl/opt_algebraic.cpp \ >>>> glsl/opt_array_splitting.cpp \ >>>> - glsl/opt_conditional_discard.cpp \ >>>> glsl/opt_constant_folding.cpp \ >>>> glsl/opt_constant_propagation.cpp \ >>>> glsl/opt_constant_variable.cpp \ >>>> diff --git a/src/compiler/glsl/glsl_parser_extras.cpp >>>> b/src/compiler/glsl/glsl_parser_extras.cpp >>>> index 307e0d6215..fe25c23ccc 100644 >>>> --- a/src/compiler/glsl/glsl_parser_extras.cpp >>>> +++ b/src/compiler/glsl/glsl_parser_extras.cpp >>>> @@ -2139,7 +2139,6 @@ do_common_optimization(exec_list *ir, bool linked, >>>> } >>>> propagate_invariance(ir); >>>> OPT(do_if_simplification, ir); >>>> - OPT(opt_conditional_discard, ir); >>>> OPT(do_copy_propagation, ir); >>>> OPT(do_copy_propagation_elements, ir); >>>> >>>> diff --git a/src/compiler/glsl/ir_optimization.h >>>> b/src/compiler/glsl/ir_optimization.h >>>> index 5d57ca85fd..022b7ab12f 100644 >>>> --- a/src/compiler/glsl/ir_optimization.h >>>> +++ b/src/compiler/glsl/ir_optimization.h >>>> @@ -93,7 +93,6 @@ bool ir_constant_fold(ir_rvalue **rvalue); >>>> bool do_rebalance_tree(exec_list *instructions); >>>> bool do_algebraic(exec_list *instructions, bool native_integers, >>>> const struct gl_shader_compiler_options *options); >>>> -bool opt_conditional_discard(exec_list *instructions); >>>> bool do_constant_folding(exec_list *instructions); >>>> bool do_constant_variable(exec_list *instructions); >>>> bool do_constant_variable_unlinked(exec_list *instructions); >>>> diff --git a/src/compiler/glsl/opt_conditional_discard.cpp >>>> b/src/compiler/glsl/opt_conditional_discard.cpp >>>> deleted file mode 100644 >>>> index 6d8a23460d..0000000000 >>>> --- a/src/compiler/glsl/opt_conditional_discard.cpp >>>> +++ /dev/null >>>> @@ -1,88 +0,0 @@ >>>> -/* >>>> - * Copyright © 2014 Intel Corporation >>>> - * >>>> - * 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. >>>> - */ >>>> - >>>> -/** >>>> - * \file opt_conditional_discard.cpp >>>> - * >>>> - * Replace >>>> - * >>>> - * if (cond) discard; >>>> - * >>>> - * with >>>> - * >>>> - * (discard <condition>) >>>> - */ >>>> - >>>> -#include "compiler/glsl_types.h" >>>> -#include "ir.h" >>>> - >>>> -namespace { >>>> - >>>> -class opt_conditional_discard_visitor : public ir_hierarchical_visitor { >>>> -public: >>>> - opt_conditional_discard_visitor() >>>> - { >>>> - progress = false; >>>> - } >>>> - >>>> - ir_visitor_status visit_leave(ir_if *); >>>> - >>>> - bool progress; >>>> -}; >>>> - >>>> -} /* anonymous namespace */ >>>> - >>>> -bool >>>> -opt_conditional_discard(exec_list *instructions) >>>> -{ >>>> - opt_conditional_discard_visitor v; >>>> - v.run(instructions); >>>> - return v.progress; >>>> -} >>>> - >>>> -ir_visitor_status >>>> -opt_conditional_discard_visitor::visit_leave(ir_if *ir) >>>> -{ >>>> - /* Look for "if (...) discard" with no else clause or extra >>>> statements. */ >>>> - if (ir->then_instructions.is_empty() || >>>> - !ir->then_instructions.get_head_raw()->next->is_tail_sentinel() || >>>> - !((ir_instruction *) >>>> ir->then_instructions.get_head_raw())->as_discard() || >>>> - !ir->else_instructions.is_empty()) >>>> - return visit_continue; >>>> - >>>> - /* Move the condition and replace the ir_if with the ir_discard. */ >>>> - ir_discard *discard = (ir_discard *) >>>> ir->then_instructions.get_head_raw(); >>>> - if (!discard->condition) >>>> - discard->condition = ir->condition; >>>> - else { >>>> - void *ctx = ralloc_parent(ir); >>>> - discard->condition = new(ctx) ir_expression(ir_binop_logic_and, >>>> - ir->condition, >>>> - discard->condition); >>>> - } >>>> - ir->replace_with(discard); >>>> - >>>> - progress = true; >>>> - >>>> - return visit_continue; >>>> -} >>>> diff --git a/src/compiler/glsl/opt_if_simplification.cpp >>>> b/src/compiler/glsl/opt_if_simplification.cpp >>>> index 05159319ba..64c56d0a2f 100644 >>>> --- a/src/compiler/glsl/opt_if_simplification.cpp >>>> +++ b/src/compiler/glsl/opt_if_simplification.cpp >>>> @@ -44,23 +44,12 @@ public: >>>> } >>>> >>>> ir_visitor_status visit_leave(ir_if *); >>>> - ir_visitor_status visit_enter(ir_assignment *); >>>> >>>> bool made_progress; >>>> }; >>>> >>>> } /* unnamed namespace */ >>>> >>>> -/* We only care about the top level "if" instructions, so don't >>>> - * descend into expressions. >>>> - */ >>>> -ir_visitor_status >>>> -ir_if_simplification_visitor::visit_enter(ir_assignment *ir) >>>> -{ >>>> - (void) ir; >>>> - return visit_continue_with_parent; >>>> -} >>> >>> I don't understand this change. What's going on? >>> >>> (Everything else in the series looks good. I've made a few trivial >>> changes locally -- whitespace mostly) >> >> As far as I'm aware, the series is blocked waiting on this. > > Oh, wow! I'm really sorry for that one. I really need to get my > email filter working again, cause I keep missing these mails :/ > > Quite frankly, I don't understand this change either. > Looking back at it now, the only idea that pops to mind is that > opt_conditional_discard did not have this hook in it. > So the two passes do not follow the exact same pattern, > so to not introduce a functional change I removed it. > I don't see a good reason for this change though, so I'm > currently running some tests with this part in, > just to be on the safe side. I'll keep you posted.
The code that was removed was an optimization. This pass only cares about if-statements. Since assignments cannot contain if-statements, don't descend into it. An assignment can also not contain a discard, so we could probably apply the same optimization to the conditional-discard pass. I'd suggest a first patch that adds the optimization to opt_conditional_discard.cpp followed by a patch that merges the two while leaving the optimization intact. > _______________________________________________ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev