Re: [PATCH] Remove inefficient branchless conditional negate optimization

[Jeff Law <law at redhat dot com>]

On 03/04/15 09:18, Wilco Dijkstra wrote:
> > Jeff Law wrote:
> > On 02/26/15 10:30, Wilco Dijkstra wrote:
> > > Several GCC versions ago a conditional negate optimization was introduced as a workaround
> > for
> > > PR45685. However the branchless expansion for conditional negate is extremely inefficient on
> > most
> > > targets (5 sequentially dependent instructions rather than 2 on AArch64). Since the
> > underlying issue
> > > has been resolved (the example in PR45685 no longer generates a branch on x64), remove the
> > > workaround so that conditional negates are treated in exactly the same way as conditional
> > invert,
> > > add, subtract, and, orr, xor etc. Simple example:
> > >
> > > int f(int x) { if (x > 3) x = -x; return x; }
> > You need to bootstrap and regression test the change before it can be
> > approved.
> >
> > You should turn this little example into a testcase.  It's fine with me
> > if this new test is ARM specific.
> >
> >
> > You should also find a way to change the test gcc.dg/tree-ssa/pr45685.c
> > in such a way that it ensures there aren't any undesirable branches.
> >
> > I've got enough history to know this is fixing a regression of sorts for
> > the ARM platform.  So once the issues above are addressed it can go
> > forward even without a BZ noting the regression.
>
> I updated the x64 testcase to explicitly check for conditional move (which
> was simpler than checking for unnecessary branches). Also add a new testcase
> for AArch64 to ensure we continue to emit csneg. Bootstrapped & regression
> tested on AArch64 and x64.
>
> ChangeLog:
> 2015-03-04  Wilco Dijkstra  <wdijkstr@arm.com>
>
>          * gcc/tree-ssa-phiopt.c (neg_replacement): Remove.
>          (tree_ssa_phiopt_worker): Remove negate optimization.
>          * gcc/testsuite/gcc.dg/tree-ssa/pr45685.c: Update test case
>          to check for conditional move on x64.
>          * gcc/testsuite/gcc.target/aarch64/csneg-1.c (test_csneg_cmp):
>          New test.
Can you move pr45685.c into gcc.target/i386?

I know Richi said next stage1, but given this fixes a performance 
regression for ARM and it's reverting rather than adding new code, I 
think this is OK for the trunk with the testcase moved.

So, OK with the testcase moved into gcc.target/i386/

jeff