Re: [PATCH][RTL-ifcvt] Improve conditional select ops on immediates

[Kyrill Tkachov <kyrylo.tkachov@arm.com>]

On 03/08/15 14:37, Uros Bizjak wrote:
> On Mon, Aug 3, 2015 at 3:02 PM, Kyrill Tkachov <kyrylo.tkachov@arm.com> wrote:
>> On 03/08/15 13:33, Uros Bizjak wrote:
>>> Hello!
>>>
>>>> 2015-07-30  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>>>>
>>>>       * ifcvt.c (noce_try_store_flag_constants): Make logic of the case
>>>>       when diff == STORE_FLAG_VALUE or diff == -STORE_FLAG_VALUE more
>>>>       explicit.  Prefer to add the flag whenever possible.
>>>>       (noce_process_if_block): Try noce_try_store_flag_constants before
>>>>       noce_try_cmove.
>>>>
>>>> 2015-07-30  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>>>>
>>>>       * gcc.target/aarch64/csel_bfx_1.c: New test.
>>>>       * gcc.target/aarch64/csel_imms_inc_1.c: Likewise.
>>> This patch regressed following tests on x86_64:
>>>
>>> FAIL: gcc.target/i386/cmov2.c scan-assembler sbb
>>> FAIL: gcc.target/i386/cmov3.c scan-assembler cmov[^3]
>>
>> Sorry for that :(
>> I could have sworn they passed for me during my run...
>>
>>> While cmov3 case is questionable by itself in light of PR56309 [1],
>>> the cnov2 case regressed from:
>>>
>>>          cmpl    %edi, %esi
>>>          sbbl    %eax, %eax
>>>          andl    $-10, %eax
>>>          addl    $5, %eax
>>>          ret
>>>
>>> to:
>>>
>>>          xorl    %eax, %eax
>>>          cmpl    %esi, %edi
>>>          setbe   %al
>>>          negl    %eax
>>>          andl    $10, %eax
>>>          subl    $5, %eax
>>>          ret
>>>
>>> Please note that sbb (aka {*x86_movsicc_0_m1} ) is not generated
>>> anymore. Non-QImode setcc instructions are problematic on x86, since
>>> they need to be zero-extended to their full length.
> IMO, we can check if the target is able to generate insn that
> implements conditional move between 0 and -1 for certain comparison
> mode, like:
>
> #(insn:TI 27 26 28 2 (parallel [
> #            (set (reg:SI 0 ax [orig:87 D.1845 ] [87])
> #                (if_then_else:SI (ltu:SI (reg:CC 17 flags)
> #                        (const_int 0 [0]))
> #                    (const_int -1 [0xffffffffffffffff])
> #                    (const_int 0 [0])))
> #            (clobber (reg:CC 17 flags))
> #        ]) cmov2.c:9 947 {*x86_movsicc_0_m1}
> #     (expr_list:REG_DEAD (reg:CC 17 flags)
> #        (expr_list:REG_UNUSED (reg:CC 17 flags)
> #            (nil))))
>         sbbl    %eax, %eax      # 27    *x86_movsicc_0_m1       [length = 2]
>
> this is the key insn, and as shown above, further asm sequence is
> similar between patched and unpatched compiler.

Hmm yes.
We have a HAVE_conditional_move check, but that's not fine grained enough.
How about trying an emit_conditional_move and checking that the result is a
single insn?

Kyrill


>
> Uros.
>