RE: [PATCH][GCC 9] arm: Fix ICEs with compare-and-swap and -march=armv8-m.base [PR99977]

[Kyrylo Tkachov <Kyrylo.Tkachov@arm.com>]

> -----Original Message-----
> From: Alex Coplan <Alex.Coplan@arm.com>
> Sent: Thursday, March 31, 2022 5:20 PM
> To: gcc-patches@gcc.gnu.org
> Cc: Richard Earnshaw <Richard.Earnshaw@arm.com>; Kyrylo Tkachov
> <Kyrylo.Tkachov@arm.com>
> Subject: [PATCH][GCC 9] arm: Fix ICEs with compare-and-swap and -
> march=armv8-m.base [PR99977]
> 
> Hi,
> 
> This is a backport of the fix for PR99977 to the GCC 9 branch. The only
> case where the GCC 10 patch did not apply cleanly was on sync.md, where
> some of the context has changed, but the substance of the patch has not
> changed, it simply required applying by hand.
> 
> Tested as follows:
>  - Bootstrap/regtest on arm-linux-gnueabihf.
>  - Regression tested a cross compiler configured with
>    --with-arch=armv8-m.base.
> 
> OK for the GCC 9 branch?
> 

Ok.
Thanks,
Kyrill

> Thanks,
> Alex
> 
> ---
> 
> The PR shows two ICEs with __sync_bool_compare_and_swap and
> -mcpu=cortex-m23 (equivalently, -march=armv8-m.base): one in LRA and
> one
> later on, after the CAS insn is split.
> 
> The LRA ICE occurs because the
> @atomic_compare_and_swap<CCSI:arch><SIDI:mode>_1 pattern attempts
> to tie
> two output operands together (operands 0 and 1 in the third
> alternative). LRA can't handle this, since it doesn't make sense for an
> insn to assign to the same operand twice.
> 
> The later (post-splitting) ICE occurs because the expansion of the
> cbranchsi4_scratch insn doesn't quite go according to plan. As it
> stands, arm_split_compare_and_swap calls gen_cbranchsi4_scratch,
> attempting to pass a register (neg_bval) to use as a scratch register.
> However, since the RTL template has a match_scratch here,
> gen_cbranchsi4_scratch ignores this argument and produces a scratch rtx.
> Since this is all happening after RA, this is doomed to fail (and we get
> an ICE about the insn not matching its constraints).
> 
> It seems that the motivation for the choice of constraints in the
> atomic_compare_and_swap pattern comes from an attempt to satisfy the
> constraints of the cbranchsi4_scratch insn. This insn requires the
> scratch register to be the same as the input register in the case that
> we use a larger negative immediate (one that satisfies J, but not L).
> 
> Of course, as noted above, LRA refuses to assign two output operands to
> the same register, so this was never going to work.
> 
> The solution I'm proposing here is to collapse the alternatives to the
> CAS insn (allowing the two output register operands to be matched to
> different registers) and to ensure that the constraints for
> cbranchsi4_scratch are met in arm_split_compare_and_swap. We do this by
> inserting a move to ensure the source and destination registers match if
> necessary (i.e. in the case of large negative immediates).
> 
> Another notable change here is that we only do:
> 
>   emit_move_insn (neg_bval, const1_rtx);
> 
> for non-negative immediates. This is because the ADDS instruction used in
> the negative case suffices to leave a suitable value in neg_bval: if the
> operands compare equal, we don't take the branch (so neg_bval will be
> set by the load exclusive). Otherwise, the ADDS will leave a nonzero
> value in neg_bval, which will correctly signal that the CAS has failed
> when it is later negated.
> 
> gcc/ChangeLog:
> 
>         PR target/99977
>         * config/arm/arm.c (arm_split_compare_and_swap): Fix up codegen
>         with negative immediates: ensure we expand cbranchsi4_scratch
>         correctly and ensure we satisfy its constraints.
>         * config/arm/sync.md
>         (@atomic_compare_and_swap<CCSI:arch><NARROW:mode>_1): Don't
>         attempt to tie two output operands together with constraints;
>         collapse two alternatives.
>         (@atomic_compare_and_swap<CCSI:arch><SIDI:mode>_1): Likewise.
>         * config/arm/thumb1.md (cbranchsi4_neg_late): New.
> 
> gcc/testsuite/ChangeLog:
> 
>         PR target/99977
>         * gcc.target/arm/pr99977.c: New test.