PR90724 - ICE with __sync_bool_compare_and_swap with -march=armv8.2-a

Message ID CAAgBjMnMqXAnbtnTT4Q_0GDFw9X3mRfbActR1bMkU73Cg_x6Yg@mail.gmail.com
State New
Headers show
Series
  • PR90724 - ICE with __sync_bool_compare_and_swap with -march=armv8.2-a
Related show

Commit Message

Prathamesh Kulkarni July 10, 2019, 11:24 a.m.
Hi,
For following test-case,
static long long AL[24];

int
check_ok (void)
{
  return (__sync_bool_compare_and_swap (AL+1, 0x200000003ll, 0x1234567890ll));
}

Compiling with -O2 -march=armv8.2-a results in:
pr90724.c: In function ‘check_ok’:
pr90724.c:7:1: error: unrecognizable insn:
    7 | }
      | ^
(insn 11 10 12 2 (set (reg:CC 66 cc)
        (compare:CC (reg:DI 95)
            (const_int 8589934595 [0x200000003]))) "pr90724.c":6:11 -1
     (nil))

IIUC, the issue is that 0x200000003 falls outside the range of
allowable immediate in cmp ? If it's replaced by a small constant then it works.

The ICE results with -march=armv8.2-a because, we enter if
(TARGET_LSE) { ... } condition
in aarch64_expand_compare_and_swap, while with -march=armv8.a it goes into else,
which forces oldval into register if the predicate fails to match.

The attached patch checks if y (oldval) satisfies aarch64_plus_operand
predicate and if not, forces it to be in register, which resolves ICE.
Does it look OK ?

Bootstrap+testing in progress on aarch64-linux-gnu.

PS: The issue has nothing to do with SVE, which I incorrectly
mentioned in bug report.

Thanks,
Prathamesh
2019-07-10  Prathamesh Kulkarni  <prathamesh.kulkarni@linaro.org>

	PR target/90724
	* config/aarch64/aarch64.c (aarch64_gen_compare_reg_maybe_ze): Force y
	in reg if it fails aarch64_plus_operand predicate.

Comments

Prathamesh Kulkarni July 17, 2019, 7:17 a.m. | #1
On Wed, 10 Jul 2019 at 16:54, Prathamesh Kulkarni
<prathamesh.kulkarni@linaro.org> wrote:
>

> Hi,

> For following test-case,

> static long long AL[24];

>

> int

> check_ok (void)

> {

>   return (__sync_bool_compare_and_swap (AL+1, 0x200000003ll, 0x1234567890ll));

> }

>

> Compiling with -O2 -march=armv8.2-a results in:

> pr90724.c: In function ‘check_ok’:

> pr90724.c:7:1: error: unrecognizable insn:

>     7 | }

>       | ^

> (insn 11 10 12 2 (set (reg:CC 66 cc)

>         (compare:CC (reg:DI 95)

>             (const_int 8589934595 [0x200000003]))) "pr90724.c":6:11 -1

>      (nil))

>

> IIUC, the issue is that 0x200000003 falls outside the range of

> allowable immediate in cmp ? If it's replaced by a small constant then it works.

>

> The ICE results with -march=armv8.2-a because, we enter if

> (TARGET_LSE) { ... } condition

> in aarch64_expand_compare_and_swap, while with -march=armv8.a it goes into else,

> which forces oldval into register if the predicate fails to match.

>

> The attached patch checks if y (oldval) satisfies aarch64_plus_operand

> predicate and if not, forces it to be in register, which resolves ICE.

> Does it look OK ?

>

> Bootstrap+testing in progress on aarch64-linux-gnu.

ping https://gcc.gnu.org/ml/gcc-patches/2019-07/msg00793.html

Thanks,
Prathamesh
>

> PS: The issue has nothing to do with SVE, which I incorrectly

> mentioned in bug report.

>

> Thanks,

> Prathamesh
Kyrill Tkachov July 17, 2019, 8:15 a.m. | #2
Hi Prathamesh

On 7/10/19 12:24 PM, Prathamesh Kulkarni wrote:
> Hi,

> For following test-case,

> static long long AL[24];

>

> int

> check_ok (void)

> {

>   return (__sync_bool_compare_and_swap (AL+1, 0x200000003ll, 

> 0x1234567890ll));

> }

>

> Compiling with -O2 -march=armv8.2-a results in:

> pr90724.c: In function ‘check_ok’:

> pr90724.c:7:1: error: unrecognizable insn:

>     7 | }

>       | ^

> (insn 11 10 12 2 (set (reg:CC 66 cc)

>         (compare:CC (reg:DI 95)

>             (const_int 8589934595 [0x200000003]))) "pr90724.c":6:11 -1

>      (nil))

>

> IIUC, the issue is that 0x200000003 falls outside the range of

> allowable immediate in cmp ? If it's replaced by a small constant then 

> it works.

>

> The ICE results with -march=armv8.2-a because, we enter if

> (TARGET_LSE) { ... } condition

> in aarch64_expand_compare_and_swap, while with -march=armv8.a it goes 

> into else,

> which forces oldval into register if the predicate fails to match.

>

> The attached patch checks if y (oldval) satisfies aarch64_plus_operand

> predicate and if not, forces it to be in register, which resolves ICE.

> Does it look OK ?

>

> Bootstrap+testing in progress on aarch64-linux-gnu.

>

> PS: The issue has nothing to do with SVE, which I incorrectly

> mentioned in bug report.

>

This looks ok to me (but you'll need maintainer approval).

Does this fail on the branches as well?

Thanks,

Kyrill


> Thanks,

> Prathamesh
Prathamesh Kulkarni July 17, 2019, 12:45 p.m. | #3
On Wed, 17 Jul 2019 at 13:45, Kyrill Tkachov
<kyrylo.tkachov@foss.arm.com> wrote:
>

> Hi Prathamesh

>

> On 7/10/19 12:24 PM, Prathamesh Kulkarni wrote:

> > Hi,

> > For following test-case,

> > static long long AL[24];

> >

> > int

> > check_ok (void)

> > {

> >   return (__sync_bool_compare_and_swap (AL+1, 0x200000003ll,

> > 0x1234567890ll));

> > }

> >

> > Compiling with -O2 -march=armv8.2-a results in:

> > pr90724.c: In function ‘check_ok’:

> > pr90724.c:7:1: error: unrecognizable insn:

> >     7 | }

> >       | ^

> > (insn 11 10 12 2 (set (reg:CC 66 cc)

> >         (compare:CC (reg:DI 95)

> >             (const_int 8589934595 [0x200000003]))) "pr90724.c":6:11 -1

> >      (nil))

> >

> > IIUC, the issue is that 0x200000003 falls outside the range of

> > allowable immediate in cmp ? If it's replaced by a small constant then

> > it works.

> >

> > The ICE results with -march=armv8.2-a because, we enter if

> > (TARGET_LSE) { ... } condition

> > in aarch64_expand_compare_and_swap, while with -march=armv8.a it goes

> > into else,

> > which forces oldval into register if the predicate fails to match.

> >

> > The attached patch checks if y (oldval) satisfies aarch64_plus_operand

> > predicate and if not, forces it to be in register, which resolves ICE.

> > Does it look OK ?

> >

> > Bootstrap+testing in progress on aarch64-linux-gnu.

> >

> > PS: The issue has nothing to do with SVE, which I incorrectly

> > mentioned in bug report.

> >

> This looks ok to me (but you'll need maintainer approval).

>

> Does this fail on the branches as well?

Hi Kyrill,
Thanks for the review. The test also fails on gcc-9-branch (but not on gcc-8).

Thanks,
Prathamesh
>

> Thanks,

>

> Kyrill

>

>

> > Thanks,

> > Prathamesh

Patch

diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index a18fbd0f0aa..22d4726e19a 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -1930,6 +1930,9 @@  aarch64_gen_compare_reg_maybe_ze (RTX_CODE code, rtx x, rtx y,
 	}
     }
 
+  if (!aarch64_plus_operand (y, y_mode))
+    y = force_reg (y_mode, y);
+
   return aarch64_gen_compare_reg (code, x, y);
 }