On Wed, Mar 4, 2015 at 9:10 AM, Jeff Law <l...@redhat.com> wrote:
> 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 <wdijk...@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/
>
Condition on pr45685.c should be target { ! ia32 }, not
target x86_64-*-*.
--
H.J.
