[patch tree-optimization]: Improve handling of conditional-branches on targets with high branch costs

[Kai Tietz]

2011/10/26 Jiangning Liu <jiangning.liu@arm.com>:
>
>
>> -----Original Message-----
>> From: gcc-patches-owner@gcc.gnu.org [mailto:gcc-patches-
>> owner@gcc.gnu.org] On Behalf Of Michael Matz
>> Sent: Tuesday, October 11, 2011 10:45 PM
>> To: Kai Tietz
>> Cc: Richard Guenther; Kai Tietz; gcc-patches@gcc.gnu.org; Richard
>> Henderson
>> Subject: Re: [patch tree-optimization]: Improve handling of
>> conditional-branches on targets with high branch costs
>>
>> Hi,
>>
>> On Tue, 11 Oct 2011, Kai Tietz wrote:
>>
>> > > Better make it a separate function the first tests your new
>> > > conditions, and then calls simple_operand_p.
>> >
>> > Well, either I make it a new function and call it instead of
>> > simple_operand_p,
>>
>> That's what I meant, yes.
>>
>> > >> @@ -5149,13 +5176,6 @@ fold_truthop (location_t loc, enum tree_
>> > >>                          build2 (BIT_IOR_EXPR, TREE_TYPE (ll_arg),
>> > >>                                  ll_arg, rl_arg),
>> > >>                          build_int_cst (TREE_TYPE (ll_arg), 0));
>> > >> -
>> > >> -      if (LOGICAL_OP_NON_SHORT_CIRCUIT)
>> > >> -     {
>> > >> -       if (code != orig_code || lhs != orig_lhs || rhs !=
>> orig_rhs)
>> > >> -         return build2_loc (loc, code, truth_type, lhs, rhs);
>> > >> -       return NULL_TREE;
>> > >> -     }
>> > >
>> > > Why do you remove this hunk?  Shouldn't you instead move the hunk
>> you
>> > > added to fold_truth_andor() here.  I realize this needs some TLC to
>> > > fold_truth_andor_1, because right now it early-outs for non-
>> comparisons,
>> > > but it seems the better place.  I.e. somehow move the below code
>> into the
>> > > above branch, with the associated diddling on fold_truth_andor_1
>> that it
>> > > gets called.
>> >
>> > This hunk is removed, as it is vain to do here.
>>
>> There is a fallthrough now, that wasn't there before.  I don't know if
>> it's harmless, I just wanted to mention it.
>>
>
> Yes, this part introduced different behavior for this small case,
>
> int f(char *i, int j)
> {
>        if (*i && j!=2)
>                return *i;
>        else
>                return j;
> }
>
> Before the fix, we have
>
>  D.4710 = *i;
>  D.4711 = D.4710 != 0;
>  D.4712 = j != 2;
>  D.4713 = D.4711 & D.4712;
>  if (D.4713 != 0) goto <D.4714>; else goto <D.4715>;
>  <D.4714>:
>  D.4710 = *i;
>  D.4716 = (int) D.4710;
>  return D.4716;
>  <D.4715>:
>  D.4716 = j;
>  return D.4716;
>
> After the fix, we have
>
>  D.4711 = *i;
>  if (D.4711 != 0) goto <D.4712>; else goto <D.4710>;
>  <D.4712>:
>  if (j != 2) goto <D.4713>; else goto <D.4710>;
>  <D.4713>:
>  D.4711 = *i;
>  D.4714 = (int) D.4711;
>  return D.4714;
>  <D.4710>:
>  D.4714 = j;
>  return D.4714;
>
> Does this meet the original expectation?
>
> I find the code below in function fold_truth_andor makes difference,
>
>      /* Transform (A AND-IF B) into (A AND B), or (A OR-IF B)   into (A OR B).
>         For sequence point consistancy, we need to check for trapping,
>         and side-effects.  */
>      else if (code == icode && simple_operand_p_2 (arg0)
>               && simple_operand_p_2 (arg1))
>         return fold_build2_loc (loc, ncode, type, arg0, arg1);
>
> for "*i != 0" simple_operand_p(*i) returns false. Originally this is not checked by the code. I don't see the patch originally wanted to cover this. Can this be explained reasonably?
>
> I'm not arguing this patch did worse thing, but only want to understand the rationale behind this and justify this patch doesn't hurt anything else. Actually on the contrary, I measured and this change accidently made some benchmarks significantly improved due to some other reasons.
>
> Thanks,
> -Jiangning
>
>> > Btw richi asked for it, and I agree that new TRUTH-AND/OR packing is
>> > better done at a single place in fold_truth_andor only.
>>
>> As fold_truthop is called twice by fold_truth_andor, the latter might
>> indeed be the better place.
>>
>>
>> Ciao,
>> Michael.

Well, as far as I understand C specification and sequence points, it
makes indeed a difference to do branch-cost optimization on
instructions might cause a signal traps. As signal could be handled in
specifc handlers. You need to consider here that short-circuit
optimization assumes associative law on operands.  So right-hand side
might be evaluaded before the left-hand-side.  And this indeed breaks
IMHO the sequences as mentioned in C specification about conditions.
 So a memory de-referencing (same as a floating-point trap) can never
be treated as simple, as far as I understood this.  So this patch -
beside improving logic for branch-cost merging - fixes this latent
issue.

Regards,
Kai