[Mesa-dev] [PATCH 3/4] glsl: Merge if-simplification and conditional discard optimization
Thomas Helland
thomashelland90 at gmail.com
Wed May 17 18:58:52 UTC 2017
2017-05-16 18:42 GMT+02:00 Ian Romanick <idr at freedesktop.org>:
> On 05/16/2017 05:35 AM, Thomas Helland wrote:
>> 2017-05-16 3:31 GMT+02:00 Matt Turner <mattst88 at gmail.com>:
>>> On Mon, Apr 24, 2017 at 4:50 PM, Matt Turner <mattst88 at gmail.com> wrote:
>>>> On Thu, Apr 6, 2017 at 12:49 PM, Thomas Helland
>>>> <thomashelland90 at 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.
>
Yeah, adding it to the conditional discard pass should not be an issue.
Added the optimization back, and there's no change in shader-db.
Although, my shader-db seems to be segfaulting after the
ivy-bridge float64 stuff landed. so I'm not able to run the whole thing.
> 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.
>
I can do that. Give me a couple hours, and I'll have two new
patches on here.
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
>
More information about the mesa-dev
mailing list