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

[Joey Ye <joey.ye.cc@gmail.com>]

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