Richard Biener <richard.guent...@gmail.com> writes:
>>>> diff --git a/gcc/fold-const.c b/gcc/fold-const.c
>>>> index de45a2c..7f00e72 100644
>>>> --- a/gcc/fold-const.c
>>>> +++ b/gcc/fold-const.c
>>>> @@ -319,7 +318,7 @@ fold_overflow_warning (const char* gmsgid, enum
>>> warn_strict_overflow_code wc)
>>>> /* Return true if the built-in mathematical function specified by CODE
>>>> is odd, i.e. -f(x) == f(-x). */
>>>>
>>>> -static bool
>>>> +bool
>>>> negate_mathfn_p (enum built_in_function code)
>>>> {
>>>> switch (code)
>>>
>>> Belongs more to builtins.[ch] if exported.
>>
>> The long-term plan is to abstract away whether it's a built-in function
>> or an internal function, in which case I hope to have a single predicate
>> that handles both. I'm not sure where the code should go after that change.
>> Maybe a new file?
>
> Hmm, we'll see. So just leave it in fold-const.c for now.
OK, thanks.
>>>> diff --git a/gcc/fold-const.h b/gcc/fold-const.h
>>>> index ee74dc8..4d5b24b 100644
>>>> --- a/gcc/fold-const.h
>>>> +++ b/gcc/fold-const.h
>>>> @@ -173,6 +173,7 @@ extern tree sign_bit_p (tree, const_tree);
>>>> extern tree exact_inverse (tree, tree);
>>>> extern tree const_unop (enum tree_code, tree, tree);
>>>> extern tree const_binop (enum tree_code, tree, tree, tree);
>>>> +extern bool negate_mathfn_p (enum built_in_function);
>>>>
>>>> /* Return OFF converted to a pointer offset type suitable as offset for
>>>> POINTER_PLUS_EXPR. Use location LOC for this conversion. */
>>>> diff --git a/gcc/passes.def b/gcc/passes.def
>>>> index dc3f44c..36d2b3b 100644
>>>> --- a/gcc/passes.def
>>>> +++ b/gcc/passes.def
>>>> @@ -159,6 +159,7 @@ along with GCC; see the file COPYING3. If not see
>>>> /* After CCP we rewrite no longer addressed locals into SSA
>>>> form if possible. */
>>>> NEXT_PASS (pass_complete_unrolli);
>>>> + NEXT_PASS (pass_backprop);
>>>> NEXT_PASS (pass_phiprop);
>>>> NEXT_PASS (pass_forwprop);
>>>
>>> Any reason to not put this later? I was thinking before reassoc.
>>
>> I think we're relying on FRE to notice the redundancy in the
>> builtins-*.c tests, once this pass has converted the version
>> with redundant sign ops to make it look like the version without.
>> reassoc is likely to be too late.
>
> There is PRE after reassoc (run as FRE at -O1).
Ah, OK. It looks like that also runs after sincos though, whereas
I think we want an FRE between this pass and sincos.
>> I also thought it should go before rather than after some instance
>> of forwprop because the pass might expose more forward folding
>> opportunities. E.g. if the sign of A = -B * B doesn't matter,
>> we'll end up with A = B * B, which might be foldable with uses of A.
>> It seems less likely that forwprop would expose more backprop
>> opportunities.
>
> Indeed. I was asking because backprop runs after inlining but
> with nearly no effective scalar cleanup after it to cleanup after
> inlining.
>
> In principle it only depends on some kind of DCE (DSE?) to avoid
> false uses, right?
Yeah, that sounds right. Complex control-flow leading up to uses
shouldn't be a problem, but phantom uses would be a blocker.
>
> It's probably ok where you put it, I just wanted to get an idea of your
> reasoning.
>
>>
>>>> +/* Make INFO describe all uses of RHS in ASSIGN. */
>>>> +
>>>> +void
>>>> +backprop::process_assign_use (gassign *assign, tree rhs, usage_info *info)
>>>> +{
>>>> + tree lhs = gimple_assign_lhs (assign);
>>>> + switch (gimple_assign_rhs_code (assign))
>>>> + {
>>>> + case ABS_EXPR:
>>>> + /* The sign of the input doesn't matter. */
>>>> + info->flags.ignore_sign = true;
>>>> + break;
>>>> +
>>>> + case COND_EXPR:
>>>> + /* For A = B ? C : D, propagate information about all uses of A
>>>> + to B and C. */
>>>> + if (rhs != gimple_assign_rhs1 (assign))
>>>> + if (const usage_info *lhs_info = lookup_operand (lhs))
>>>
>>> Use && instead of nested if
>>
>> That means introducing an extra level of braces just for something
>> that that isn't needed by the first statement, i.e.:
>>
>> {
>> const usage_info *lhs_info;
>> if (rhs != gimple_assign_rhs1 (assign)
>> && (lhs_info = lookup_operand (lhs)))
>> *info = *lhs_info;
>> break;
>> }
>>
>> There also used to be a strong preference for not embedding assignments
>> in && and || conditions.
>>
>> If there had been some other set-up for the lookup_operand call, we
>> would have had:
>>
>> if (rhs != gimple_assign_rhs1 (assign))
>> {
>> ...
>> if (const usage_info *lhs_info = lookup_operand (lhs))
>> ..
>> }
>>
>> and presumably that would have been OK. So if the original really isn't,
>> acceptable, I'd rather write it as:
>>
>> if (rhs != gimple_assign_rhs1 (assign))
>> {
>> const usage_info *lhs_info = lookup_operand (lhs);
>> if (lhs_info)
>> ..
>> }
>>
>> I thought we'd embraced the idea of declarations in if conditions though.
>
> I missed the init in if ... yeah, there doesn't seem to be a good way
> though I prefer {} around the inner if at least which means the last
> form would be my reference.
OK.
>>>> +/* Process all uses of VAR and record or update the result in
>>>> + M_INFO_MAP and M_VARS. */
>>>> +
>>>> +void
>>>> +backprop::process_var (tree var)
>>>> +{
>>>> + if (has_zero_uses (var))
>>>> + return;
>>>> +
>>>> + usage_info info;
>>>> + intersect_uses (var, &info);
>>>
>>> It feels somewhat backward that we do a backward walk over all stmts
>>> (thus processing all use-stmts before defs) but then "forward" process
>>> uses of defs. Wouldn't it be easier(?) to "intersect" incrementally
>>> by processing only uses for all stmts we walk? Or is this about
>>> avoiding to allocate a 'info' for DEFs that end up with no interesting
>>> info?
>>
>> Yeah, that was the idea. In practice very few SSA names end up
>> with any useful information.
>
> Ok, let's add a comment somewhere noting that and the choice of
> "backward forward propagation".
Will do. I think I'll also rework it to be lazy, which should be
more efficient as well as more obvious. Not sure why I didn't do
that in the first place.
>>>> +/* Strip all sign operations from the rvalue at *RHS_PTR in STMT.
>>>> + Return true if something changed. The caller is responsible
>>>> + for the necessary bookkeeping. */
>>>> +
>>>> +static bool
>>>> +strip_sign_op (gimple *stmt, tree *rhs_ptr)
>>>> +{
>>>> + if (tree new_rhs = strip_sign_op (*rhs_ptr))
>>>> + {
>>>> + if (dump_file && (dump_flags & TDF_DETAILS))
>>>> + note_replacement (stmt, *rhs_ptr, new_rhs);
>>>> + *rhs_ptr = new_rhs;
>>>
>>> So it looks you are only changing stmts when the stmt result produces
>>> the same value. Just double-checking, as otherwise you'd need to care
>>> about debug stmts ...
>>
>> No, it can change values, like the case you saw later for phis.
>> This applies to all the cases where the optimisation depends on the
>> propagated info for the lhs, rather than being inherent to the operation.
>> So e.g. we can change the value of A in A = B * C, if all uses of A
>> don't care about the sign.
>>
>> At the moment the only change we can make is that the result could be
>> the negative of its original value.
>
> Ok, so then there is the debug issue. Consider
>
> x = ...;
>
> which you change the sign for. The user in gdb when printing 'x' needs to
> see the original value or "optimized out", not the negated value. This means
> you have to replace the LHS of the stmt with a new SSA name which is
> best done (with proper debug effects) by removing the original stmt and
> replacing all uses of the LHS with the new stmt lhs. Note that the
> same is true for all derived values, so even if you only change
>
> _1 = ...;
>
> (no user visible value) then a derived value
>
>
> x_2 = _1 + 2;
>
> might change sign.
>
> Well, you have to think about it at least ;)
OK, will fix.
Thanks,
Richard
