Uros Bizjak <ubiz...@gmail.com> writes:
> Hello!
>>> gcc/testsuite/
>>> Changelog for gcc/testsuite/Changelog
>>> 2018-08-14 Vlad Lazar <vlad.la...@arm.com>
>>>
>>> * gcc.target/aarch64/imm_choice_comparison.c: New.
>>>
>>> gcc/
>>> Changelog for gcc/Changelog
>>> 2018-08-14 Vlad Lazar <vlad.la...@arm.com>
>>> * expmed.h (canonicalize_comparison): New declaration.
>>> * expmed.c (canonicalize_comparison, equivalent_cmp_code): New.
>>> * expmed.c (emit_store_flag_1): Add call to canonicalize_comparison.
>>> * optabs.c (prepare_cmp_insn): Likewise.
>>> * rtl.h (unsigned_condition_p): New function which checks if a
>>> comparison operator is unsigned.
>> Thanks. I fixed up the ChangeLog entry and installed this on the trunk.
>>
>> Richard S -- thanks for working with Vlad to push this forward.
>
> This patch caused
>
> +FAIL: gcc.target/i386/20040112-1.c scan-assembler testb
>
> on x86 targets, reported in PR86994 [1].
>
> The compiler now expands with:
>
> (insn 12 8 13 4 (set (reg:CCGC 17 flags)
> (compare:CCGC (reg/v:QI 87 [ status ])
> (const_int -1 [0xffffffffffffffff]))) "20040112-1.c":13 9
> {*cmpqi_1}
> (nil))
>
> instead of:
>
> (insn 10 8 11 4 (set (reg:CCGOC 17 flags)
> (compare:CCGOC (reg/v:QI 87 [ status ])
> (const_int 0 [0]))) "20040112-1.c":13 5 {*cmpqi_ccno_1}
> (nil))
>
> The canonicalization, introduced in r263591 does not universally
> benefit all targets.
The patch does ask the target which immediate is cheaper though.
Even for AArch64 it's only a conditional canoicalisation: sometimes
the original value is cheaper, sometimes the new one is.
The problem in this case is that insn_cost says:
(set (reg:QI X) (const_int 0))
has a cost of 4 but:
(set (reg:QI X) (const_int -1))
has a cost of 2. So the costs make -1 seem like the cheaper immediate.
This in turn seems to be a bad interaction between ix86_rtx_costs and:
cost = set_src_cost (SET_SRC (set), GET_MODE (SET_DEST (set)), speed);
return cost > 0 ? cost : COSTS_N_INSNS (1);
in pattern_cost. ix86_rtx_costs returns 0 for 0 and 2 for -1, where 2
is a sub-insn cost. But pattern_cost converts the 0 cost into the cost
of a full instruction, making 0 seem more expensive than -1.
I wonder whether we should change the return to:
return MAX (cost, 1);
or even:
return cost + 1;
Who knows what will break if we do that though...
This of course is why Segher added the insn_cost hook.
> I wonder why TARGET_CANONICALIZE_COMPARISON target hook was not used
> instead.
The idea is that this is a target-independent technique that needs
target-specific costs, a bit like open-coding multiplication using
shifts and adds. The code to do the transformation itself should be
target-independent and only the cost decision should be target-specific.
Thanks,
Richard
