diff mbox series

[AArch64] Fix test failure for pr84682-2.c

Message ID DB6PR0802MB2504BCFAD63BCAA484B5EE8FE7D70@DB6PR0802MB2504.eurprd08.prod.outlook.com
State New
Headers show
Series [AArch64] Fix test failure for pr84682-2.c | expand

Commit Message

Bin Cheng March 16, 2018, 11:42 a.m. UTC 
Hi,
This simple patch fixes test case failure for pr84682-2.c by returning
false on wrong mode rtx in aarch64_classify_address, rather than assert.

Bootstrap and test on aarch64.  Is it OK?

Thanks,
bin

2018-03-16  Bin Cheng  <bin.cheng@arm.com>

	* config/aarch64/aarch64.c (aarch64_classify_address): Return false
	on wrong mode rtx, rather than assert.

Comments

Kyrill Tkachov March 16, 2018, 11:48 a.m. UTC | #1 
Hi Bin,

On 16/03/18 11:42, Bin Cheng wrote:
> Hi,
> This simple patch fixes test case failure for pr84682-2.c by returning
> false on wrong mode rtx in aarch64_classify_address, rather than assert.
>
> Bootstrap and test on aarch64.  Is it OK?
>
> Thanks,
> bin
>
> 2018-03-16  Bin Cheng  <bin.cheng@arm.com>
>
>         * config/aarch64/aarch64.c (aarch64_classify_address): Return false
>         on wrong mode rtx, rather than assert.

This looks ok to me in light of https://gcc.gnu.org/ml/gcc-patches/2018-03/msg00633.html
This function is used to validate inline asm operands too, not just internally-generated addresses.
Therefore all kinds of garbage must be rejected gracefully rather than ICEing.

You'll need an approval from an AArch64 maintainer though.

Thanks,
Kyrill
Richard Sandiford March 17, 2018, 8:54 a.m. UTC | #2 
Kyrill  Tkachov <kyrylo.tkachov@foss.arm.com> writes:
> Hi Bin,
>
> On 16/03/18 11:42, Bin Cheng wrote:
>> Hi,
>> This simple patch fixes test case failure for pr84682-2.c by returning
>> false on wrong mode rtx in aarch64_classify_address, rather than assert.
>>
>> Bootstrap and test on aarch64.  Is it OK?
>>
>> Thanks,
>> bin
>>
>> 2018-03-16  Bin Cheng  <bin.cheng@arm.com>
>>
>>         * config/aarch64/aarch64.c (aarch64_classify_address): Return false
>>         on wrong mode rtx, rather than assert.
>
> This looks ok to me in light of
> https://gcc.gnu.org/ml/gcc-patches/2018-03/msg00633.html
> This function is used to validate inline asm operands too, not just
> internally-generated addresses.
> Therefore all kinds of garbage must be rejected gracefully rather than ICEing.
>
> You'll need an approval from an AArch64 maintainer though.

IMO we should make address_operand itself check something like:

  (GET_MODE (x) == VOIDmode || SCALAR_INT_MODE_P (GET_MODE (x)))

Target-independent code fundamentally assumes that an address will not
be a float, so I think the check should be in target-independent code
rather than copied to each individual backend.

This was only caught on aarch64 because we added the assert, but I think
some backends ignore the mode of the address and so would actually accept
simple float rtxes.

Thanks,
Richard
Bin.Cheng March 22, 2018, 11:07 a.m. UTC | #3 
On Sat, Mar 17, 2018 at 8:54 AM, Richard Sandiford
<richard.sandiford@linaro.org> wrote:
> Kyrill  Tkachov <kyrylo.tkachov@foss.arm.com> writes:
>> Hi Bin,
>>
>> On 16/03/18 11:42, Bin Cheng wrote:
>>> Hi,
>>> This simple patch fixes test case failure for pr84682-2.c by returning
>>> false on wrong mode rtx in aarch64_classify_address, rather than assert.
>>>
>>> Bootstrap and test on aarch64.  Is it OK?
>>>
>>> Thanks,
>>> bin
>>>
>>> 2018-03-16  Bin Cheng  <bin.cheng@arm.com>
>>>
>>>         * config/aarch64/aarch64.c (aarch64_classify_address): Return false
>>>         on wrong mode rtx, rather than assert.
>>
>> This looks ok to me in light of
>> https://gcc.gnu.org/ml/gcc-patches/2018-03/msg00633.html
>> This function is used to validate inline asm operands too, not just
>> internally-generated addresses.
>> Therefore all kinds of garbage must be rejected gracefully rather than ICEing.
>>
>> You'll need an approval from an AArch64 maintainer though.
>
> IMO we should make address_operand itself check something like:
>
>   (GET_MODE (x) == VOIDmode || SCALAR_INT_MODE_P (GET_MODE (x)))
>
> Target-independent code fundamentally assumes that an address will not
> be a float, so I think the check should be in target-independent code
> rather than copied to each individual backend.
>
> This was only caught on aarch64 because we added the assert, but I think
> some backends ignore the mode of the address and so would actually accept
> simple float rtxes.
Hi Richard,
Thanks for the suggestion generalizing the fix.  Here is the updated patch.
Bootstrap and test on x86_64 and AArch64, is it OK?

Thanks,
bin

2018-03-22  Bin Cheng  <bin.cheng@arm.com>

    * recog.c (address_operand): Return false on wrong mode for address.
    * config/aarch64/aarch64.c (aarch64_classify_address): Remove assert
    since it's checked in general code now.

>
> Thanks,
> Richard
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 07c55b1..9e965ab 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -5674,9 +5674,6 @@ aarch64_classify_address (struct aarch64_address_info *info,
       && (code != POST_INC && code != REG))
     return false;
 
-  gcc_checking_assert (GET_MODE (x) == VOIDmode
-		       || SCALAR_INT_MODE_P (GET_MODE (x)));
-
   switch (code)
     {
     case REG:
diff --git a/gcc/recog.c b/gcc/recog.c
index 0a8fa2c..510aba2 100644
--- a/gcc/recog.c
+++ b/gcc/recog.c
@@ -1070,6 +1070,11 @@ general_operand (rtx op, machine_mode mode)
 int
 address_operand (rtx op, machine_mode mode)
 {
+  /* Wrong mode for an address expr.  */
+  if (GET_MODE (op) != VOIDmode
+      && ! SCALAR_INT_MODE_P (GET_MODE (op)))
+    return false;
+
   return memory_address_p (mode, op);
 }
Bin.Cheng April 24, 2018, 2:17 p.m. UTC | #4 
On Thu, Mar 22, 2018 at 11:07 AM, Bin.Cheng <amker.cheng@gmail.com> wrote:
> On Sat, Mar 17, 2018 at 8:54 AM, Richard Sandiford
> <richard.sandiford@linaro.org> wrote:
>> Kyrill  Tkachov <kyrylo.tkachov@foss.arm.com> writes:
>>> Hi Bin,
>>>
>>> On 16/03/18 11:42, Bin Cheng wrote:
>>>> Hi,
>>>> This simple patch fixes test case failure for pr84682-2.c by returning
>>>> false on wrong mode rtx in aarch64_classify_address, rather than assert.
>>>>
>>>> Bootstrap and test on aarch64.  Is it OK?
>>>>
>>>> Thanks,
>>>> bin
>>>>
>>>> 2018-03-16  Bin Cheng  <bin.cheng@arm.com>
>>>>
>>>>         * config/aarch64/aarch64.c (aarch64_classify_address): Return false
>>>>         on wrong mode rtx, rather than assert.
>>>
>>> This looks ok to me in light of
>>> https://gcc.gnu.org/ml/gcc-patches/2018-03/msg00633.html
>>> This function is used to validate inline asm operands too, not just
>>> internally-generated addresses.
>>> Therefore all kinds of garbage must be rejected gracefully rather than ICEing.
>>>
>>> You'll need an approval from an AArch64 maintainer though.
>>
>> IMO we should make address_operand itself check something like:
>>
>>   (GET_MODE (x) == VOIDmode || SCALAR_INT_MODE_P (GET_MODE (x)))
>>
>> Target-independent code fundamentally assumes that an address will not
>> be a float, so I think the check should be in target-independent code
>> rather than copied to each individual backend.
>>
>> This was only caught on aarch64 because we added the assert, but I think
>> some backends ignore the mode of the address and so would actually accept
>> simple float rtxes.
> Hi Richard,
> Thanks for the suggestion generalizing the fix.  Here is the updated patch.
> Bootstrap and test on x86_64 and AArch64, is it OK?

Ping.

Better to have this ICE fix in GCC8?  But I guess RTL review/approval is needed.

Thanks,
bin
>
> Thanks,
> bin
>
> 2018-03-22  Bin Cheng  <bin.cheng@arm.com>
>
>     * recog.c (address_operand): Return false on wrong mode for address.
>     * config/aarch64/aarch64.c (aarch64_classify_address): Remove assert
>     since it's checked in general code now.
>
>>
>> Thanks,
>> Richard
Kyrill Tkachov May 16, 2018, 8:21 a.m. UTC | #5 
Hi Bin,


On 22/03/18 11:07, Bin.Cheng wrote:
> On Sat, Mar 17, 2018 at 8:54 AM, Richard Sandiford
> <richard.sandiford@linaro.org> wrote:
> > Kyrill  Tkachov <kyrylo.tkachov@foss.arm.com> writes:
> >> Hi Bin,
> >>
> >> On 16/03/18 11:42, Bin Cheng wrote:
> >>> Hi,
> >>> This simple patch fixes test case failure for pr84682-2.c by returning
> >>> false on wrong mode rtx in aarch64_classify_address, rather than assert.
> >>>
> >>> Bootstrap and test on aarch64.  Is it OK?
> >>>
> >>> Thanks,
> >>> bin
> >>>
> >>> 2018-03-16  Bin Cheng <bin.cheng@arm.com>
> >>>
> >>>         * config/aarch64/aarch64.c (aarch64_classify_address): Return false
> >>>         on wrong mode rtx, rather than assert.
> >>
> >> This looks ok to me in light of
> >> https://gcc.gnu.org/ml/gcc-patches/2018-03/msg00633.html
> >> This function is used to validate inline asm operands too, not just
> >> internally-generated addresses.
> >> Therefore all kinds of garbage must be rejected gracefully rather than ICEing.
> >>
> >> You'll need an approval from an AArch64 maintainer though.
> >
> > IMO we should make address_operand itself check something like:
> >
> >   (GET_MODE (x) == VOIDmode || SCALAR_INT_MODE_P (GET_MODE (x)))
> >
> > Target-independent code fundamentally assumes that an address will not
> > be a float, so I think the check should be in target-independent code
> > rather than copied to each individual backend.
> >
> > This was only caught on aarch64 because we added the assert, but I think
> > some backends ignore the mode of the address and so would actually accept
> > simple float rtxes.
> Hi Richard,
> Thanks for the suggestion generalizing the fix.  Here is the updated patch.
> Bootstrap and test on x86_64 and AArch64, is it OK?
>

I guess you need a midend maintainer to ok this now.
CC'ing Jeff...

Thanks,
Kyrill

> Thanks,
> bin
>
> 2018-03-22  Bin Cheng  <bin.cheng@arm.com>
>
>     * recog.c (address_operand): Return false on wrong mode for address.
>     * config/aarch64/aarch64.c (aarch64_classify_address): Remove assert
>     since it's checked in general code now.
>
> >
> > Thanks,
> > Richard
Joey Ye Aug. 29, 2018, 3:50 p.m. UTC | #6 
Ping^2 for Bin

The ICE is still there annoyingly.

Thanks,
Joey

On Wed, May 16, 2018 at 9:21 AM Kyrill Tkachov
<kyrylo.tkachov@foss.arm.com> wrote:
>
> Hi Bin,
>
>
> On 22/03/18 11:07, Bin.Cheng wrote:
> > On Sat, Mar 17, 2018 at 8:54 AM, Richard Sandiford
> > <richard.sandiford@linaro.org> wrote:
> > > Kyrill  Tkachov <kyrylo.tkachov@foss.arm.com> writes:
> > >> Hi Bin,
> > >>
> > >> On 16/03/18 11:42, Bin Cheng wrote:
> > >>> Hi,
> > >>> This simple patch fixes test case failure for pr84682-2.c by returning
> > >>> false on wrong mode rtx in aarch64_classify_address, rather than assert.
> > >>>
> > >>> Bootstrap and test on aarch64.  Is it OK?
> > >>>
> > >>> Thanks,
> > >>> bin
> > >>>
> > >>> 2018-03-16  Bin Cheng <bin.cheng@arm.com>
> > >>>
> > >>>         * config/aarch64/aarch64.c (aarch64_classify_address): Return false
> > >>>         on wrong mode rtx, rather than assert.
> > >>
> > >> This looks ok to me in light of
> > >> https://gcc.gnu.org/ml/gcc-patches/2018-03/msg00633.html
> > >> This function is used to validate inline asm operands too, not just
> > >> internally-generated addresses.
> > >> Therefore all kinds of garbage must be rejected gracefully rather than ICEing.
> > >>
> > >> You'll need an approval from an AArch64 maintainer though.
> > >
> > > IMO we should make address_operand itself check something like:
> > >
> > >   (GET_MODE (x) == VOIDmode || SCALAR_INT_MODE_P (GET_MODE (x)))
> > >
> > > Target-independent code fundamentally assumes that an address will not
> > > be a float, so I think the check should be in target-independent code
> > > rather than copied to each individual backend.
> > >
> > > This was only caught on aarch64 because we added the assert, but I think
> > > some backends ignore the mode of the address and so would actually accept
> > > simple float rtxes.
> > Hi Richard,
> > Thanks for the suggestion generalizing the fix.  Here is the updated patch.
> > Bootstrap and test on x86_64 and AArch64, is it OK?
> >
>
> I guess you need a midend maintainer to ok this now.
> CC'ing Jeff...
>
> Thanks,
> Kyrill
>
> > Thanks,
> > bin
> >
> > 2018-03-22  Bin Cheng  <bin.cheng@arm.com>
> >
> >     * recog.c (address_operand): Return false on wrong mode for address.
> >     * config/aarch64/aarch64.c (aarch64_classify_address): Remove assert
> >     since it's checked in general code now.
> >
> > >
> > > Thanks,
> > > Richard
>
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 07c55b1..9e965ab 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -5674,9 +5674,6 @@ aarch64_classify_address (struct aarch64_address_info *info,
       && (code != POST_INC && code != REG))
     return false;
 
-  gcc_checking_assert (GET_MODE (x) == VOIDmode
-		       || SCALAR_INT_MODE_P (GET_MODE (x)));
-
   switch (code)
     {
     case REG:
diff --git a/gcc/recog.c b/gcc/recog.c
index 0a8fa2c..510aba2 100644
--- a/gcc/recog.c
+++ b/gcc/recog.c
@@ -1070,6 +1070,11 @@ general_operand (rtx op, machine_mode mode)
 int
 address_operand (rtx op, machine_mode mode)
 {
+  /* Wrong mode for an address expr.  */
+  if (GET_MODE (op) != VOIDmode
+      && ! SCALAR_INT_MODE_P (GET_MODE (op)))
+    return false;
+
   return memory_address_p (mode, op);
 }
Richard Sandiford Aug. 29, 2018, 6:47 p.m. UTC | #7 
Joey Ye <joey.ye.cc@gmail.com> writes:
> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> index 07c55b1..9e965ab 100644
> --- a/gcc/config/aarch64/aarch64.c
> +++ b/gcc/config/aarch64/aarch64.c
> @@ -5674,9 +5674,6 @@ aarch64_classify_address (struct aarch64_address_info *info,
>        && (code != POST_INC && code != REG))
>      return false;
>  
> -  gcc_checking_assert (GET_MODE (x) == VOIDmode
> -		       || SCALAR_INT_MODE_P (GET_MODE (x)));
> -
>    switch (code)
>      {
>      case REG:
> diff --git a/gcc/recog.c b/gcc/recog.c
> index 0a8fa2c..510aba2 100644
> --- a/gcc/recog.c
> +++ b/gcc/recog.c
> @@ -1070,6 +1070,11 @@ general_operand (rtx op, machine_mode mode)
>  int
>  address_operand (rtx op, machine_mode mode)
>  {
> +  /* Wrong mode for an address expr.  */
> +  if (GET_MODE (op) != VOIDmode
> +      && ! SCALAR_INT_MODE_P (GET_MODE (op)))
> +    return false;
> +
>    return memory_address_p (mode, op);
>  }
>  

The address_operand part is OK, thanks.

I think we should keep the assert in aarch64_classify_address, since
IMO it's a bug for anything else to reach that point.

Richard
Bin.Cheng Aug. 30, 2018, 1:02 a.m. UTC | #8 
On Thu, Aug 30, 2018 at 2:47 AM Richard Sandiford
<richard.sandiford@arm.com> wrote:
>
> Joey Ye <joey.ye.cc@gmail.com> writes:
> > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> > index 07c55b1..9e965ab 100644
> > --- a/gcc/config/aarch64/aarch64.c
> > +++ b/gcc/config/aarch64/aarch64.c
> > @@ -5674,9 +5674,6 @@ aarch64_classify_address (struct aarch64_address_info *info,
> >        && (code != POST_INC && code != REG))
> >      return false;
> >
> > -  gcc_checking_assert (GET_MODE (x) == VOIDmode
> > -                    || SCALAR_INT_MODE_P (GET_MODE (x)));
> > -
> >    switch (code)
> >      {
> >      case REG:
> > diff --git a/gcc/recog.c b/gcc/recog.c
> > index 0a8fa2c..510aba2 100644
> > --- a/gcc/recog.c
> > +++ b/gcc/recog.c
> > @@ -1070,6 +1070,11 @@ general_operand (rtx op, machine_mode mode)
> >  int
> >  address_operand (rtx op, machine_mode mode)
> >  {
> > +  /* Wrong mode for an address expr.  */
> > +  if (GET_MODE (op) != VOIDmode
> > +      && ! SCALAR_INT_MODE_P (GET_MODE (op)))
> > +    return false;
> > +
> >    return memory_address_p (mode, op);
> >  }
> >
>
> The address_operand part is OK, thanks.
>
> I think we should keep the assert in aarch64_classify_address, since
> IMO it's a bug for anything else to reach that point.

Hi Joey,
Could you help me update the patch as suggested by Richard and commit
it please?  My new assignment is still on the way.
Thanks very much!

Thanks,
bin
>
> Richard
Joey Ye Aug. 30, 2018, 10:20 a.m. UTC | #9 
Hi Bin & Richard,

It is not as simple as keeping the assertion, which still fails even
with the change in reorg.c. The testing result is as following:

I. With Bin's patch version 2 (removing the assertion in aarch64.c and
adding the check in reorg.c): pr84682-2.c passes

II. With Richard's suggestion to keep the assertion in aarch64, but
adding the check in reorg.c: pr84682-2.c fails

Apparently there is a different path that reaches the assertion.

With II:
/build/trunk/src/gcc/gcc/testsuite/gcc.dg/torture/pr84682-2.c: In
function 'b': /build/trunk/src/gcc/gcc/testsuite/gcc.dg/torture/pr84682-2.c:10:1:
internal compiler error: in aarch64_classify_address, at
config/aarch64/aarch64.c:5721
0xfa4071 aarch64_classify_address
        /build/trunk/src/gcc/gcc/config/aarch64/aarch64.c:5720
0xfa94f3 aarch64_legitimate_address_hook_p
        /build/trunk/src/gcc/gcc/config/aarch64/aarch64.c:6003
0xb85661 strict_memory_address_addr_space_p(machine_mode, rtx_def*,
unsigned char)
        /build/trunk/src/gcc/gcc/reload.c:2177
0xb75da9 constrain_operands(int, unsigned long)
        /build/trunk/src/gcc/gcc/recog.c:2706
0xb761a0 extract_constrain_insn(rtx_insn*)
        /build/trunk/src/gcc/gcc/recog.c:2210
0xa6dd57 check_rtl
        /build/trunk/src/gcc/gcc/lra.c:2182
0xa737db lra(_IO_FILE*)
        /build/trunk/src/gcc/gcc/lra.c:2616
0xa25989 do_reload
        /build/trunk/src/gcc/gcc/ira.c:5469
0xa25989 execute

Current trunk without any patch:
/build/trunk/src/gcc/gcc/testsuite/gcc.dg/torture/pr84682-2.c: In
function 'b': /build/trunk/src/gcc/gcc/testsuite/gcc.dg/torture/pr84682-2.c:9:3:
internal compiler error: in aarch64_classify_address, at
config/aarch64/aarch64.c:5721
0xfa4011 aarch64_classify_address
        /build/trunk/src/gcc/gcc/config/aarch64/aarch64.c:5720
0xfa9493 aarch64_legitimate_address_hook_p
        /build/trunk/src/gcc/gcc/config/aarch64/aarch64.c:6003
0xb74cf3 memory_address_addr_space_p(machine_mode, rtx_def*, unsigned char)
        /build/trunk/src/gcc/gcc/recog.c:1334
0xb74cf3 address_operand(rtx_def*, machine_mode)
        /build/trunk/src/gcc/gcc/recog.c:1073
0xb74cf3 asm_operand_ok(rtx_def*, char const*, char const**)
        /build/trunk/src/gcc/gcc/recog.c:1817
0x75e591 expand_asm_stmt
        /build/trunk/src/gcc/gcc/cfgexpand.c:3135
0x766d67 expand_gimple_stmt_1
        /build/trunk/src/gcc/gcc/cfgexpand.c:3572
0x766d67 expand_gimple_stmt
        /build/trunk/src/gcc/gcc/cfgexpand.c:3734
0x768ce7 expand_gimple_basic_block

More places need to be patched.

Thanks,
Joey
On Thu, Aug 30, 2018 at 2:02 AM Bin.Cheng <amker.cheng@gmail.com> wrote:
>
> On Thu, Aug 30, 2018 at 2:47 AM Richard Sandiford
> <richard.sandiford@arm.com> wrote:
> >
> > Joey Ye <joey.ye.cc@gmail.com> writes:
> > > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> > > index 07c55b1..9e965ab 100644
> > > --- a/gcc/config/aarch64/aarch64.c
> > > +++ b/gcc/config/aarch64/aarch64.c
> > > @@ -5674,9 +5674,6 @@ aarch64_classify_address (struct aarch64_address_info *info,
> > >        && (code != POST_INC && code != REG))
> > >      return false;
> > >
> > > -  gcc_checking_assert (GET_MODE (x) == VOIDmode
> > > -                    || SCALAR_INT_MODE_P (GET_MODE (x)));
> > > -
> > >    switch (code)
> > >      {
> > >      case REG:
> > > diff --git a/gcc/recog.c b/gcc/recog.c
> > > index 0a8fa2c..510aba2 100644
> > > --- a/gcc/recog.c
> > > +++ b/gcc/recog.c
> > > @@ -1070,6 +1070,11 @@ general_operand (rtx op, machine_mode mode)
> > >  int
> > >  address_operand (rtx op, machine_mode mode)
> > >  {
> > > +  /* Wrong mode for an address expr.  */
> > > +  if (GET_MODE (op) != VOIDmode
> > > +      && ! SCALAR_INT_MODE_P (GET_MODE (op)))
> > > +    return false;
> > > +
> > >    return memory_address_p (mode, op);
> > >  }
> > >
> >
> > The address_operand part is OK, thanks.
> >
> > I think we should keep the assert in aarch64_classify_address, since
> > IMO it's a bug for anything else to reach that point.
>
> Hi Joey,
> Could you help me update the patch as suggested by Richard and commit
> it please?  My new assignment is still on the way.
> Thanks very much!
>
> Thanks,
> bin
> >
> > Richard
Joey Ye Aug. 30, 2018, 10:28 a.m. UTC | #10 
typo: s/reorg.c/recog.c/g
On Thu, Aug 30, 2018 at 11:20 AM Joey Ye <joey.ye.cc@gmail.com> wrote:
>
> Hi Bin & Richard,
>
> It is not as simple as keeping the assertion, which still fails even
> with the change in reorg.c. The testing result is as following:
>
> I. With Bin's patch version 2 (removing the assertion in aarch64.c and
> adding the check in reorg.c): pr84682-2.c passes
>
> II. With Richard's suggestion to keep the assertion in aarch64, but
> adding the check in reorg.c: pr84682-2.c fails
>
> Apparently there is a different path that reaches the assertion.
>
> With II:
> /build/trunk/src/gcc/gcc/testsuite/gcc.dg/torture/pr84682-2.c: In
> function 'b': /build/trunk/src/gcc/gcc/testsuite/gcc.dg/torture/pr84682-2.c:10:1:
> internal compiler error: in aarch64_classify_address, at
> config/aarch64/aarch64.c:5721
> 0xfa4071 aarch64_classify_address
>         /build/trunk/src/gcc/gcc/config/aarch64/aarch64.c:5720
> 0xfa94f3 aarch64_legitimate_address_hook_p
>         /build/trunk/src/gcc/gcc/config/aarch64/aarch64.c:6003
> 0xb85661 strict_memory_address_addr_space_p(machine_mode, rtx_def*,
> unsigned char)
>         /build/trunk/src/gcc/gcc/reload.c:2177
> 0xb75da9 constrain_operands(int, unsigned long)
>         /build/trunk/src/gcc/gcc/recog.c:2706
> 0xb761a0 extract_constrain_insn(rtx_insn*)
>         /build/trunk/src/gcc/gcc/recog.c:2210
> 0xa6dd57 check_rtl
>         /build/trunk/src/gcc/gcc/lra.c:2182
> 0xa737db lra(_IO_FILE*)
>         /build/trunk/src/gcc/gcc/lra.c:2616
> 0xa25989 do_reload
>         /build/trunk/src/gcc/gcc/ira.c:5469
> 0xa25989 execute
>
> Current trunk without any patch:
> /build/trunk/src/gcc/gcc/testsuite/gcc.dg/torture/pr84682-2.c: In
> function 'b': /build/trunk/src/gcc/gcc/testsuite/gcc.dg/torture/pr84682-2.c:9:3:
> internal compiler error: in aarch64_classify_address, at
> config/aarch64/aarch64.c:5721
> 0xfa4011 aarch64_classify_address
>         /build/trunk/src/gcc/gcc/config/aarch64/aarch64.c:5720
> 0xfa9493 aarch64_legitimate_address_hook_p
>         /build/trunk/src/gcc/gcc/config/aarch64/aarch64.c:6003
> 0xb74cf3 memory_address_addr_space_p(machine_mode, rtx_def*, unsigned char)
>         /build/trunk/src/gcc/gcc/recog.c:1334
> 0xb74cf3 address_operand(rtx_def*, machine_mode)
>         /build/trunk/src/gcc/gcc/recog.c:1073
> 0xb74cf3 asm_operand_ok(rtx_def*, char const*, char const**)
>         /build/trunk/src/gcc/gcc/recog.c:1817
> 0x75e591 expand_asm_stmt
>         /build/trunk/src/gcc/gcc/cfgexpand.c:3135
> 0x766d67 expand_gimple_stmt_1
>         /build/trunk/src/gcc/gcc/cfgexpand.c:3572
> 0x766d67 expand_gimple_stmt
>         /build/trunk/src/gcc/gcc/cfgexpand.c:3734
> 0x768ce7 expand_gimple_basic_block
>
> More places need to be patched.
>
> Thanks,
> Joey
> On Thu, Aug 30, 2018 at 2:02 AM Bin.Cheng <amker.cheng@gmail.com> wrote:
> >
> > On Thu, Aug 30, 2018 at 2:47 AM Richard Sandiford
> > <richard.sandiford@arm.com> wrote:
> > >
> > > Joey Ye <joey.ye.cc@gmail.com> writes:
> > > > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> > > > index 07c55b1..9e965ab 100644
> > > > --- a/gcc/config/aarch64/aarch64.c
> > > > +++ b/gcc/config/aarch64/aarch64.c
> > > > @@ -5674,9 +5674,6 @@ aarch64_classify_address (struct aarch64_address_info *info,
> > > >        && (code != POST_INC && code != REG))
> > > >      return false;
> > > >
> > > > -  gcc_checking_assert (GET_MODE (x) == VOIDmode
> > > > -                    || SCALAR_INT_MODE_P (GET_MODE (x)));
> > > > -
> > > >    switch (code)
> > > >      {
> > > >      case REG:
> > > > diff --git a/gcc/recog.c b/gcc/recog.c
> > > > index 0a8fa2c..510aba2 100644
> > > > --- a/gcc/recog.c
> > > > +++ b/gcc/recog.c
> > > > @@ -1070,6 +1070,11 @@ general_operand (rtx op, machine_mode mode)
> > > >  int
> > > >  address_operand (rtx op, machine_mode mode)
> > > >  {
> > > > +  /* Wrong mode for an address expr.  */
> > > > +  if (GET_MODE (op) != VOIDmode
> > > > +      && ! SCALAR_INT_MODE_P (GET_MODE (op)))
> > > > +    return false;
> > > > +
> > > >    return memory_address_p (mode, op);
> > > >  }
> > > >
> > >
> > > The address_operand part is OK, thanks.
> > >
> > > I think we should keep the assert in aarch64_classify_address, since
> > > IMO it's a bug for anything else to reach that point.
> >
> > Hi Joey,
> > Could you help me update the patch as suggested by Richard and commit
> > it please?  My new assignment is still on the way.
> > Thanks very much!
> >
> > Thanks,
> > bin
> > >
> > > Richard
Richard Sandiford Aug. 30, 2018, 12:24 p.m. UTC | #11 
Joey Ye <joey.ye.cc@gmail.com> writes:
> Hi Bin & Richard,
>
> It is not as simple as keeping the assertion, which still fails even
> with the change in reorg.c. The testing result is as following:
>
> I. With Bin's patch version 2 (removing the assertion in aarch64.c and
> adding the check in reorg.c): pr84682-2.c passes
>
> II. With Richard's suggestion to keep the assertion in aarch64, but
> adding the check in reorg.c: pr84682-2.c fails
>
> Apparently there is a different path that reaches the assertion.

Yeah, looks like we also need to make constrain_operands check
address_operand for 'p' (which I think it should do irrespective
of "strict", since in general we can only reload an operand as
a pointer if the original value has the right form for an address).

Thanks,
Richard
Richard Earnshaw (lists) Dec. 12, 2018, 11:29 a.m. UTC | #12 
On 30/08/2018 13:24, Richard Sandiford wrote:
> Joey Ye <joey.ye.cc@gmail.com> writes:
>> Hi Bin & Richard,
>>
>> It is not as simple as keeping the assertion, which still fails even
>> with the change in reorg.c. The testing result is as following:
>>
>> I. With Bin's patch version 2 (removing the assertion in aarch64.c and
>> adding the check in reorg.c): pr84682-2.c passes
>>
>> II. With Richard's suggestion to keep the assertion in aarch64, but
>> adding the check in reorg.c: pr84682-2.c fails
>>
>> Apparently there is a different path that reaches the assertion.
> 
> Yeah, looks like we also need to make constrain_operands check
> address_operand for 'p' (which I think it should do irrespective
> of "strict", since in general we can only reload an operand as
> a pointer if the original value has the right form for an address).
> 
> Thanks,
> Richard
> 

What's the status of this patch?

R.
diff mbox series

Patch

diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 07c55b1..8790902 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -5674,8 +5674,10 @@  aarch64_classify_address (struct aarch64_address_info *info,
       && (code != POST_INC && code != REG))
     return false;
 
-  gcc_checking_assert (GET_MODE (x) == VOIDmode
-		       || SCALAR_INT_MODE_P (GET_MODE (x)));
+  /* Wrong mode for an address expr.  */
+  if (GET_MODE (x) != VOIDmode
+      && ! SCALAR_INT_MODE_P (GET_MODE (x)))
+    return false;
 
   switch (code)
     {