Re: [PATCH][AArch64] PR target/69161: Don't use special predicate for CCmode comparisons in expressions that require matching modes

[James Greenhalgh <james.greenhalgh@arm.com>]

On Fri, Jan 29, 2016 at 02:27:34PM +0000, Kyrill Tkachov wrote:
> Hi all,
> 
> In this PR we ICE during combine when trying to propagate a comparison into a vec_duplicate,
> that is we end up creating the rtx:
> (vec_duplicate:V4SI (eq:CC_NZ (reg:CC_NZ 66 cc)
>         (const_int 0 [0])))
> 
> The documentation for vec_duplicate says:
> "The output vector mode must have the same submodes as the input vector mode or the scalar modes"
> So this is invalid RTL, which triggers an assert in simplify-rtx to that effect.
> 
> It has been suggested on the PR that this is because we use a special_predicate for
> aarch64_comparison_operator which means that it ignores the mode when matching.
> This is fine when used in RTXes that don't need it, like if_then_else expressions
> but can cause trouble when used in places where the modes do matter, like in
> SET operations. In this particular ICE the cause was the conditional store
> patterns that could end up matching an intermediate rtx during combine of
> (set (reg:SI) (eq:CC_NZ x y)).
> 
> The suggested solution is to define a separate predicate with the same
> conditions as aarch64_comparison_operator but make it not special, so it gets
> automatic mode checks to prevent such a situation.
> 
> This patch does that.
> Bootstrapped and tested on aarch64-linux-gnu.
> SPEC2006 codegen did not change with this patch, so there shouldn't be
> any code quality regressions.
> 
> Ok for trunk?

It would be good to leave a more detailed comment on
"aarch64_comparison_operator_mode" as to why we need it.

Otherwise, this is OK.

Thanks,
James

> 
> Thanks,
> Kyrill
> 
> 2016-01-29  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
> 
>     PR target/69161
>     * config/aarch64/predicates.md (aarch64_comparison_operator_mode):
>     New predicate.
>     (aarch64_comparison_operator): Break overly long line into two.
>     (aarch64_comparison_operation): Likewise.
>     * config/aarch64/aarch64.md (cstorecc4): Use
>     aarch64_comparison_operator_mode instead of
>     aarch64_comparison_operator.
>     (cstore<mode>4): Likewise.
>     (aarch64_cstore<mode>): Likewise.
>     (*cstoresi_insn_uxtw): Likewise.
>     (cstore<mode>_neg): Likewise.
>     (*cstoresi_neg_uxtw): Likewise.
> 
> 2016-01-29  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
> 
>     PR target/69161
>     * gcc.c-torture/compile/pr69161.c: New test.