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

["Jiangning Liu" <jiangning dot liu at arm dot 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.