This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] Fix x86 setcc + movzbl peephole2s (PR target/86314)
- From: Uros Bizjak <ubizjak at gmail dot com>
- To: Jakub Jelinek <jakub at redhat dot com>
- Cc: "gcc-patches at gcc dot gnu dot org" <gcc-patches at gcc dot gnu dot org>
- Date: Tue, 26 Jun 2018 13:03:06 +0200
- Subject: Re: [PATCH] Fix x86 setcc + movzbl peephole2s (PR target/86314)
- References: <20180626105223.GO7166@tucnak>
On Tue, Jun 26, 2018 at 12:52 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> Hi!
>
> These peephole2s assume that when matching insns like:
> [(parallel [(set (reg FLAGS_REG) (match_operand 0))
> (match_operand 4)])
> that operands[4] must be a set of a register with operands[0]
> as SET_SRC, but that isn't the case e.g. for:
> (insn 9 8 10 2 (parallel [
> (set (reg:CCC 17 flags)
> (compare:CCC (unspec_volatile:DI [
> (mem/v:DI (reg/v/f:DI 5 di [orig:94 p ] [94]) [-1 S8 A64])
> (const_int 5 [0x5])
> ] UNSPECV_XCHG)
> (const_int 0 [0])))
> (set (zero_extract:DI (mem/v:DI (reg/v/f:DI 5 di [orig:94 p ] [94]) [-1 S8 A64])
> (const_int 1 [0x1])
> (reg:DI 0 ax [96]))
> (const_int 1 [0x1]))
> ]) "pr86314.C":7 5268 {atomic_bit_test_and_setdi_1}
> (expr_list:REG_DEAD (reg/v/f:DI 5 di [orig:94 p ] [94])
> (expr_list:REG_DEAD (reg:DI 0 ax [96])
> (nil))))
>
> As we want to clear operands[3] before this instruction, we need to
> verify that it isn't used in operands[0] (we check that) and that
> operands[4] does not set it (also checked), but also need to check that
> operands[4] doesn't use it (which we don't do). In the usual case
> when SET_DEST of operands[4] is a REG and SET_SRC is operands[0] it
> makes no difference, but if SET_DEST sets something that uses operands[3]
> in it, or if SET_SRC is different from operands[0] and uses operands[3],
> then this results in a wrong peephole2 transformation.
>
> Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for
> trunk and release branches?
>
> 2018-06-26 Jakub Jelinek <jakub@redhat.com>
>
> PR target/86314
> * config/i386/i386.md (setcc + movzbl to xor + setcc peephole2s):
> Check reg_overlap_mentioned_p in addition to reg_set_p with the same
> operands.
>
> * gcc.dg/pr86314.c: New test.
OK everywhere.
Thanks,
Uros.
> --- gcc/config/i386/i386.md.jj 2018-06-25 14:51:24.878990827 +0200
> +++ gcc/config/i386/i386.md 2018-06-26 10:01:43.042907384 +0200
> @@ -12761,6 +12761,7 @@ (define_peephole2
> "(peep2_reg_dead_p (3, operands[1])
> || operands_match_p (operands[1], operands[3]))
> && ! reg_overlap_mentioned_p (operands[3], operands[0])
> + && ! reg_overlap_mentioned_p (operands[3], operands[4])
> && ! reg_set_p (operands[3], operands[4])
> && peep2_regno_dead_p (0, FLAGS_REG)"
> [(parallel [(set (match_dup 5) (match_dup 0))
> @@ -12786,6 +12787,7 @@ (define_peephole2
> || operands_match_p (operands[2], operands[4]))
> && ! reg_overlap_mentioned_p (operands[4], operands[0])
> && ! reg_overlap_mentioned_p (operands[4], operands[1])
> + && ! reg_overlap_mentioned_p (operands[4], operands[5])
> && ! reg_set_p (operands[4], operands[5])
> && refers_to_regno_p (FLAGS_REG, operands[1], (rtx *)NULL)
> && peep2_regno_dead_p (0, FLAGS_REG)"
> @@ -12835,6 +12837,7 @@ (define_peephole2
> "(peep2_reg_dead_p (3, operands[1])
> || operands_match_p (operands[1], operands[3]))
> && ! reg_overlap_mentioned_p (operands[3], operands[0])
> + && ! reg_overlap_mentioned_p (operands[3], operands[4])
> && ! reg_set_p (operands[3], operands[4])
> && peep2_regno_dead_p (0, FLAGS_REG)"
> [(parallel [(set (match_dup 5) (match_dup 0))
> @@ -12861,6 +12864,7 @@ (define_peephole2
> || operands_match_p (operands[2], operands[4]))
> && ! reg_overlap_mentioned_p (operands[4], operands[0])
> && ! reg_overlap_mentioned_p (operands[4], operands[1])
> + && ! reg_overlap_mentioned_p (operands[4], operands[5])
> && ! reg_set_p (operands[4], operands[5])
> && refers_to_regno_p (FLAGS_REG, operands[1], (rtx *)NULL)
> && peep2_regno_dead_p (0, FLAGS_REG)"
> --- gcc/testsuite/gcc.dg/pr86314.c.jj 2018-06-26 10:25:35.190160477 +0200
> +++ gcc/testsuite/gcc.dg/pr86314.c 2018-06-26 10:24:42.310077331 +0200
> @@ -0,0 +1,20 @@
> +// PR target/86314
> +// { dg-do run { target sync_int_long } }
> +// { dg-options "-O2" }
> +
> +__attribute__((noinline, noclone)) unsigned long
> +foo (unsigned long *p)
> +{
> + unsigned long m = 1UL << ((*p & 1) ? 1 : 0);
> + unsigned long n = __atomic_fetch_or (p, m, __ATOMIC_SEQ_CST);
> + return (n & m) == 0;
> +}
> +
> +int
> +main ()
> +{
> + unsigned long v = 1;
> + if (foo (&v) != 1)
> + __builtin_abort ();
> + return 0;
> +}
>
> Jakub