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
Prathamesh Kulkarni July 25, 2019, 6:26 a.m. | #4
On Wed, 17 Jul 2019 at 18:15, Prathamesh Kulkarni
<prathamesh.kulkarni@linaro.org> wrote:
>

> 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).

Hi James,
Is the patch OK to commit  ?
https://gcc.gnu.org/ml/gcc-patches/2019-07/msg00793.html

Thanks,
Prathamesh
>

> Thanks,

> Prathamesh

> >

> > Thanks,

> >

> > Kyrill

> >

> >

> > > Thanks,

> > > Prathamesh
Prathamesh Kulkarni Aug. 1, 2019, 10:04 a.m. | #5
On Thu, 25 Jul 2019 at 11:56, Prathamesh Kulkarni
<prathamesh.kulkarni@linaro.org> wrote:
>

> On Wed, 17 Jul 2019 at 18:15, Prathamesh Kulkarni

> <prathamesh.kulkarni@linaro.org> wrote:

> >

> > 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).

> Hi James,

> Is the patch OK to commit  ?

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

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

Thanks,
Prathamesh
>

> Thanks,

> Prathamesh

> >

> > Thanks,

> > Prathamesh

> > >

> > > Thanks,

> > >

> > > Kyrill

> > >

> > >

> > > > Thanks,

> > > > Prathamesh
Prathamesh Kulkarni Aug. 8, 2019, 5:52 a.m. | #6
On Thu, 1 Aug 2019 at 15:34, Prathamesh Kulkarni
<prathamesh.kulkarni@linaro.org> wrote:
>

> On Thu, 25 Jul 2019 at 11:56, Prathamesh Kulkarni

> <prathamesh.kulkarni@linaro.org> wrote:

> >

> > On Wed, 17 Jul 2019 at 18:15, Prathamesh Kulkarni

> > <prathamesh.kulkarni@linaro.org> wrote:

> > >

> > > 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).

> > Hi James,

> > Is the patch OK to commit  ?

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

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

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

Thanks,
Prathamesh
>

> Thanks,

> Prathamesh

> >

> > Thanks,

> > Prathamesh

> > >

> > > Thanks,

> > > Prathamesh

> > > >

> > > > Thanks,

> > > >

> > > > Kyrill

> > > >

> > > >

> > > > > Thanks,

> > > > > Prathamesh
Prathamesh Kulkarni Aug. 15, 2019, 1:11 p.m. | #7
On Thu, 8 Aug 2019 at 11:22, Prathamesh Kulkarni
<prathamesh.kulkarni@linaro.org> wrote:
>

> On Thu, 1 Aug 2019 at 15:34, Prathamesh Kulkarni

> <prathamesh.kulkarni@linaro.org> wrote:

> >

> > On Thu, 25 Jul 2019 at 11:56, Prathamesh Kulkarni

> > <prathamesh.kulkarni@linaro.org> wrote:

> > >

> > > On Wed, 17 Jul 2019 at 18:15, Prathamesh Kulkarni

> > > <prathamesh.kulkarni@linaro.org> wrote:

> > > >

> > > > 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).

> > > Hi James,

> > > Is the patch OK to commit  ?

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

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

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

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

Thanks,
Prathamesh
>

> Thanks,

> Prathamesh

> >

> > Thanks,

> > Prathamesh

> > >

> > > Thanks,

> > > Prathamesh

> > > >

> > > > Thanks,

> > > > Prathamesh

> > > > >

> > > > > Thanks,

> > > > >

> > > > > Kyrill

> > > > >

> > > > >

> > > > > > Thanks,

> > > > > > Prathamesh
James Greenhalgh Aug. 19, 2019, 4:44 p.m. | #8
On Thu, Aug 15, 2019 at 02:11:25PM +0100, Prathamesh Kulkarni wrote:
> On Thu, 8 Aug 2019 at 11:22, Prathamesh Kulkarni

> <prathamesh.kulkarni@linaro.org> wrote:

> >

> > On Thu, 1 Aug 2019 at 15:34, Prathamesh Kulkarni

> > <prathamesh.kulkarni@linaro.org> wrote:

> > >

> > > On Thu, 25 Jul 2019 at 11:56, Prathamesh Kulkarni

> > > <prathamesh.kulkarni@linaro.org> wrote:

> > > >

> > > > On Wed, 17 Jul 2019 at 18:15, Prathamesh Kulkarni

> > > > <prathamesh.kulkarni@linaro.org> wrote:

> > > > >

> > > > > 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).

> > > > Hi James,

> > > > Is the patch OK to commit  ?

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

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

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

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


Hi,

Sorry, this missed my filters as it didn't mention AArch64 in the subject
line.

Thais is good for trunk, thanks for waiting.

James
Prathamesh Kulkarni Aug. 21, 2019, 6:35 p.m. | #9
On Mon, 19 Aug 2019 at 22:14, James Greenhalgh <james.greenhalgh@arm.com> wrote:
>

> On Thu, Aug 15, 2019 at 02:11:25PM +0100, Prathamesh Kulkarni wrote:

> > On Thu, 8 Aug 2019 at 11:22, Prathamesh Kulkarni

> > <prathamesh.kulkarni@linaro.org> wrote:

> > >

> > > On Thu, 1 Aug 2019 at 15:34, Prathamesh Kulkarni

> > > <prathamesh.kulkarni@linaro.org> wrote:

> > > >

> > > > On Thu, 25 Jul 2019 at 11:56, Prathamesh Kulkarni

> > > > <prathamesh.kulkarni@linaro.org> wrote:

> > > > >

> > > > > On Wed, 17 Jul 2019 at 18:15, Prathamesh Kulkarni

> > > > > <prathamesh.kulkarni@linaro.org> wrote:

> > > > > >

> > > > > > 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).

> > > > > Hi James,

> > > > > Is the patch OK to commit  ?

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

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

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

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

>

> Hi,

>

> Sorry, this missed my filters as it didn't mention AArch64 in the subject

> line.

>

> Thais is good for trunk, thanks for waiting.

Thanks, committed to trunk in r274805.
Is this OK to backport to gcc-9-branch ?

Thanks,
Prathamesh
>

> James

>
Prathamesh Kulkarni Sept. 9, 2019, 7:06 p.m. | #10
On Thu, 22 Aug 2019 at 00:05, Prathamesh Kulkarni
<prathamesh.kulkarni@linaro.org> wrote:
>

> On Mon, 19 Aug 2019 at 22:14, James Greenhalgh <james.greenhalgh@arm.com> wrote:

> >

> > On Thu, Aug 15, 2019 at 02:11:25PM +0100, Prathamesh Kulkarni wrote:

> > > On Thu, 8 Aug 2019 at 11:22, Prathamesh Kulkarni

> > > <prathamesh.kulkarni@linaro.org> wrote:

> > > >

> > > > On Thu, 1 Aug 2019 at 15:34, Prathamesh Kulkarni

> > > > <prathamesh.kulkarni@linaro.org> wrote:

> > > > >

> > > > > On Thu, 25 Jul 2019 at 11:56, Prathamesh Kulkarni

> > > > > <prathamesh.kulkarni@linaro.org> wrote:

> > > > > >

> > > > > > On Wed, 17 Jul 2019 at 18:15, Prathamesh Kulkarni

> > > > > > <prathamesh.kulkarni@linaro.org> wrote:

> > > > > > >

> > > > > > > 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).

> > > > > > Hi James,

> > > > > > Is the patch OK to commit  ?

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

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

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

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

> >

> > Hi,

> >

> > Sorry, this missed my filters as it didn't mention AArch64 in the subject

> > line.

> >

> > Thais is good for trunk, thanks for waiting.

> Thanks, committed to trunk in r274805.

> Is this OK to backport to gcc-9-branch ?

ping ? Would it be OK to backport this patch to gcc-9-branch ?

Thanks,
Prathamesh
>

> Thanks,

> Prathamesh

> >

> > James

> >

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);
 }