* [PATCH AArch64]Fix test failure for pr84682-2.c
@ 2018-03-16 11:43 Bin Cheng
2018-03-16 11:53 ` Kyrill Tkachov
0 siblings, 1 reply; 13+ messages in thread
From: Bin Cheng @ 2018-03-16 11:43 UTC (permalink / raw)
To: gcc-patches; +Cc: nd
[-- Attachment #1: Type: text/plain, Size: 369 bytes --]
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.
[-- Attachment #2: patch-pr84682-2.txt --]
[-- Type: text/plain, Size: 585 bytes --]
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)
{
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH AArch64]Fix test failure for pr84682-2.c
2018-03-16 11:43 [PATCH AArch64]Fix test failure for pr84682-2.c Bin Cheng
@ 2018-03-16 11:53 ` Kyrill Tkachov
2018-03-17 9:20 ` Richard Sandiford
0 siblings, 1 reply; 13+ messages in thread
From: Kyrill Tkachov @ 2018-03-16 11:53 UTC (permalink / raw)
To: Bin Cheng, gcc-patches; +Cc: James Greenhalgh, Richard Earnshaw (lists)
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH AArch64]Fix test failure for pr84682-2.c
2018-03-16 11:53 ` Kyrill Tkachov
@ 2018-03-17 9:20 ` Richard Sandiford
2018-03-22 11:11 ` Bin.Cheng
0 siblings, 1 reply; 13+ messages in thread
From: Richard Sandiford @ 2018-03-17 9:20 UTC (permalink / raw)
To: Kyrill Tkachov
Cc: Bin Cheng, gcc-patches, James Greenhalgh, Richard Earnshaw (lists)
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH AArch64]Fix test failure for pr84682-2.c
2018-03-17 9:20 ` Richard Sandiford
@ 2018-03-22 11:11 ` Bin.Cheng
2018-04-24 15:14 ` Bin.Cheng
2018-05-16 8:37 ` Kyrill Tkachov
0 siblings, 2 replies; 13+ messages in thread
From: Bin.Cheng @ 2018-03-22 11:11 UTC (permalink / raw)
To: Kyrill Tkachov, gcc-patches, James Greenhalgh,
Richard Earnshaw (lists),
Richard Sandiford
[-- Attachment #1: Type: text/plain, Size: 1888 bytes --]
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
[-- Attachment #2: patch-pr84682-2-20180319.txt --]
[-- Type: text/plain, Size: 872 bytes --]
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);
}
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH AArch64]Fix test failure for pr84682-2.c
2018-03-22 11:11 ` Bin.Cheng
@ 2018-04-24 15:14 ` Bin.Cheng
2018-05-16 8:37 ` Kyrill Tkachov
1 sibling, 0 replies; 13+ messages in thread
From: Bin.Cheng @ 2018-04-24 15:14 UTC (permalink / raw)
To: gcc-patches, Richard Earnshaw (lists), Richard Sandiford
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH AArch64]Fix test failure for pr84682-2.c
2018-03-22 11:11 ` Bin.Cheng
2018-04-24 15:14 ` Bin.Cheng
@ 2018-05-16 8:37 ` Kyrill Tkachov
2018-08-29 15:50 ` Joey Ye
1 sibling, 1 reply; 13+ messages in thread
From: Kyrill Tkachov @ 2018-05-16 8:37 UTC (permalink / raw)
To: Bin.Cheng, Kyrill Tkachov, gcc-patches, James Greenhalgh,
Richard Earnshaw, Richard Sandiford
Cc: Jeff Law
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH AArch64]Fix test failure for pr84682-2.c
2018-05-16 8:37 ` Kyrill Tkachov
@ 2018-08-29 15:50 ` Joey Ye
2018-08-29 18:47 ` Richard Sandiford
0 siblings, 1 reply; 13+ messages in thread
From: Joey Ye @ 2018-08-29 15:50 UTC (permalink / raw)
To: kyrylo.tkachov
Cc: Bin.Cheng, gcc-patches, James Greenhalgh, Richard Earnshaw,
richard.sandiford, Jeff Law
[-- Attachment #1: Type: text/plain, Size: 2406 bytes --]
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
>
[-- Attachment #2: patch-pr84682-2-20180319.txt --]
[-- Type: text/plain, Size: 872 bytes --]
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);
}
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH AArch64]Fix test failure for pr84682-2.c
2018-08-29 15:50 ` Joey Ye
@ 2018-08-29 18:47 ` Richard Sandiford
2018-08-30 1:02 ` Bin.Cheng
0 siblings, 1 reply; 13+ messages in thread
From: Richard Sandiford @ 2018-08-29 18:47 UTC (permalink / raw)
To: Joey Ye
Cc: kyrylo.tkachov, Bin.Cheng, gcc-patches, James Greenhalgh,
Richard Earnshaw, Jeff Law
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH AArch64]Fix test failure for pr84682-2.c
2018-08-29 18:47 ` Richard Sandiford
@ 2018-08-30 1:02 ` Bin.Cheng
2018-08-30 10:21 ` Joey Ye
0 siblings, 1 reply; 13+ messages in thread
From: Bin.Cheng @ 2018-08-30 1:02 UTC (permalink / raw)
To: Joey Ye, gcc-patches List, Richard Sandiford
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH AArch64]Fix test failure for pr84682-2.c
2018-08-30 1:02 ` Bin.Cheng
@ 2018-08-30 10:21 ` Joey Ye
2018-08-30 10:28 ` Joey Ye
2018-08-30 12:24 ` Richard Sandiford
0 siblings, 2 replies; 13+ messages in thread
From: Joey Ye @ 2018-08-30 10:21 UTC (permalink / raw)
To: Bin.Cheng; +Cc: gcc-patches, richard.sandiford
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH AArch64]Fix test failure for pr84682-2.c
2018-08-30 10:21 ` Joey Ye
@ 2018-08-30 10:28 ` Joey Ye
2018-08-30 12:24 ` Richard Sandiford
1 sibling, 0 replies; 13+ messages in thread
From: Joey Ye @ 2018-08-30 10:28 UTC (permalink / raw)
To: Bin.Cheng; +Cc: gcc-patches, richard.sandiford
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH AArch64]Fix test failure for pr84682-2.c
2018-08-30 10:21 ` Joey Ye
2018-08-30 10:28 ` Joey Ye
@ 2018-08-30 12:24 ` Richard Sandiford
2018-12-12 11:29 ` Richard Earnshaw (lists)
1 sibling, 1 reply; 13+ messages in thread
From: Richard Sandiford @ 2018-08-30 12:24 UTC (permalink / raw)
To: Joey Ye; +Cc: Bin.Cheng, gcc-patches
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH AArch64]Fix test failure for pr84682-2.c
2018-08-30 12:24 ` Richard Sandiford
@ 2018-12-12 11:29 ` Richard Earnshaw (lists)
0 siblings, 0 replies; 13+ messages in thread
From: Richard Earnshaw (lists) @ 2018-12-12 11:29 UTC (permalink / raw)
To: Joey Ye, Bin.Cheng, gcc-patches, richard.sandiford
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2018-12-12 11:29 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-16 11:43 [PATCH AArch64]Fix test failure for pr84682-2.c Bin Cheng
2018-03-16 11:53 ` Kyrill Tkachov
2018-03-17 9:20 ` Richard Sandiford
2018-03-22 11:11 ` Bin.Cheng
2018-04-24 15:14 ` Bin.Cheng
2018-05-16 8:37 ` Kyrill Tkachov
2018-08-29 15:50 ` Joey Ye
2018-08-29 18:47 ` Richard Sandiford
2018-08-30 1:02 ` Bin.Cheng
2018-08-30 10:21 ` Joey Ye
2018-08-30 10:28 ` Joey Ye
2018-08-30 12:24 ` Richard Sandiford
2018-12-12 11:29 ` Richard Earnshaw (lists)
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).