Re: Ping^2: [PATCH]Remove duplicate check on BRANCH_COST in fold-const.c

[Richard Guenther <richard dot guenther at gmail dot com>]

On Tue, Sep 18, 2012 at 11:32 AM, Bin Cheng <bin.cheng@arm.com> wrote:
> Ping.

I already approved your original patch upthread.

Richard.

>> -----Original Message-----
>> From: gcc-patches-owner@gcc.gnu.org [mailto:gcc-patches-owner@gcc.gnu.org]
> On
>> Behalf Of Bin.Cheng
>> Sent: Tuesday, September 04, 2012 11:20 PM
>> To: Richard Guenther; gcc-patches@gcc.gnu.org
>> Cc: Richard Earnshaw
>> Subject: Re: Ping^2: [PATCH]Remove duplicate check on BRANCH_COST in fold-
>> const.c
>>
>> Sorry, I mis-sent this offline.
>>
>> On Tue, Sep 4, 2012 at 11:00 PM, Bin.Cheng <amker.cheng@gmail.com> wrote:
>> >>>
>> >>> It's not ok (I btw fail to see the patch in this thread).  The
>> >>> current way LOGICAL_OP_NON_SHORT_CIRCUIT is implemented/used should
>> >>> instead be changed to always match the pattern
>> >>>
>> >>>       LOGICAL_OP_NON_SHORT_CIRCUIT
>> >>>       && (BRANCH_COST (optimize_function_for_speed_p (cfun),
>> >>>                     false) >= 2)
>> >>>
>> >>> and the default value of LOGICAL_OP_NON_SHORT_CIRCUIT should be 1,
>> >>> defined in defaults.h (and the docs updated).
>> >>>
>> >>
>> >> That's not going to work for modern ARM cores.  We want to set
>> >> BRANCH_COST to 1 but still have it generate the non-short-circuit
>> >> code (because conditional compares are really cheap.
>> >>
>> >
>> > Hi Richard,
>> > For now, LOGICAL_OP_NON_SHORT_CIRCUIT macro is defined as below, which
>> > is duplicate of the BRANCH_COST condition.
>> >
>> > #ifndef LOGICAL_OP_NON_SHORT_CIRCUIT
>> > #define LOGICAL_OP_NON_SHORT_CIRCUIT \
>> >   (BRANCH_COST (optimize_function_for_speed_p (cfun), \
>> >                 false) >= 2)
>> > #endif
>> >
>> > Recently we measured performance on some ARM processors and found it
>> > would be better to have non-short-circuit optimization while setting
>> > BRANCH_COST to 1, which is impossible with present codes. So here
>> > comes this patch as below:
>> >
>> > Index: gcc/fold-const.c
>> > ===================================================================
>> > --- gcc/fold-const.c    (revision 189835)
>> > +++ gcc/fold-const.c    (working copy)
>> > @@ -8443,9 +8443,7 @@
>> >    if ((tem = fold_truth_andor_1 (loc, code, type, arg0, arg1)) != 0)
>> >      return tem;
>> >
>> > -  if ((BRANCH_COST (optimize_function_for_speed_p (cfun),
>> > -                   false) >= 2)
>> > -      && LOGICAL_OP_NON_SHORT_CIRCUIT
>> > +  if (LOGICAL_OP_NON_SHORT_CIRCUIT
>> >        && (code == TRUTH_AND_EXPR
>> >            || code == TRUTH_ANDIF_EXPR
>> >            || code == TRUTH_OR_EXPR
>> >
>> > The purpose is to remove the duplicate check on BRANCH_COST.
>> >
>> > As Andrew pointed out that the patch may change behavior if some
>> > back-ends define the macro independent of BRANCH_COST. After looking
>> > into the code, there are two uses of the macro in fold-const.c, each
>> > controls one kind code transformation. The first use is:
>> >
>> >   else if (LOGICAL_OP_NON_SHORT_CIRCUIT
>> >            && lhs != 0 && rhs != 0
>> >            && (code == TRUTH_ANDIF_EXPR
>> >                || code == TRUTH_ORIF_EXPR)
>> >            && operand_equal_p (lhs, rhs, 0))
>> >
>> > The second one is:
>> >
>> >   if ((BRANCH_COST (optimize_function_for_speed_p (cfun),
>> >                     false) >= 2)
>> >       && LOGICAL_OP_NON_SHORT_CIRCUIT
>> >       && (code == TRUTH_AND_EXPR
>> >           || code == TRUTH_ANDIF_EXPR
>> >           || code == TRUTH_OR_EXPR
>> >           || code == TRUTH_ORIF_EXPR))
>> >
>> > I am not sure why the 2nd condition is designed in current way and
>> > haven't found any useful changelog on it.
>> > But considering back end can factor BRANCH_COST in
>> > LOGICAL_OP_NON_SHORT_CIRCUIT or not, we can conclude that the behavior
>> > will only be changed if some back-end want to control the two
>> > transformations differently. So the problem becomes whether the 2nd
>> > condition should be changed. Either way there is scenario cannot be
>> > covered.
>> >
>>
>> And for now,
>> FTR, only two targets redefine L_O_N_S_C: mips and rs6000.  Both set it to
>> zero so won't be affected by this change.
>>
>
> Hi Richard,
> I have tried to explain the change, but I am not sure whether it is agreed
> or...
>
> Thanks very much.
>
>
>