[PATCH] MIPS: Fix PR target/98491

[PATCH] MIPS: Fix PR target/98491

[Xi Ruoyao]

[-- Attachment #1: Type: text/plain, Size: 466 bytes --]

An invalid use of MSA_SUPPORTED_MODE_P is causing ICE on mips64el with -mmsa.
The detailed analysis is posted on bugzilla:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98491

The attached patch fixes this issue by handling the special case of
MSA_SUPPORTED_MODE_P explicitly.

Please keep me in CC since I'm not a subscriber.

And, I don't have GIT write access.
-- 
Xi Ruoyao <xry111@mengyan1223.wang>
School of Aerospace Science and Technology, Xidian University

[-- Attachment #2: 0001-MIPS-Fix-PR-target-98491.patch --]
[-- Type: text/x-patch, Size: 526 bytes --]

diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c
index 58e474e063d..8f80dcfada8 100644
--- a/gcc/config/mips/mips.c
+++ b/gcc/config/mips/mips.c
@@ -2381,7 +2381,7 @@ mips_symbol_insns (enum mips_symbol_type type, machine_mode mode)
 {
   /* MSA LD.* and ST.* cannot support loading symbols via an immediate
      operand.  */
-  if (MSA_SUPPORTED_MODE_P (mode))
+  if (mode != MAX_MACHINE_MODE && MSA_SUPPORTED_MODE_P (mode))
     return 0;
 
   return mips_symbol_insns_1 (type, mode) * (TARGET_MIPS16 ? 2 : 1);

Re: [PATCH] MIPS: Fix PR target/98491 (ChangeLog)

[Xi Ruoyao]

On 2021-01-01 07:29 +0800, Xi Ruoyao wrote:
> An invalid use of MSA_SUPPORTED_MODE_P is causing ICE on mips64el with -mmsa.
> The detailed analysis is posted on bugzilla:
> 
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98491
> 
> The attached patch fixes this issue by handling the special case of
> MSA_SUPPORTED_MODE_P explicitly.
> 
> Please keep me in CC since I'm not a subscriber.
> 
> And, I don't have GIT write access.

Sorry, I forgot to include the ChangeLog:

    gcc/ChangeLog:
    
    2020-12-31  Xi Ruoyao <xry111@mengyan1223.wang>
    
            PR target/98491
            * config/mips/mips.c (mips_symbol_insns): Do not use
              MSA_SUPPORTED_MODE_P if mode is MAX_MACHINE_MODE.
-- 
Xi Ruoyao <xry111@mengyan1223.wang>
School of Aerospace Science and Technology, Xidian University

Re: [PATCH] MIPS: Fix PR target/98491 (ChangeLog)

[Jeff Law]

[-- Attachment #1: Type: text/plain, Size: 1097 bytes --]



On 12/31/20 4:34 PM, Xi Ruoyao via Gcc-patches wrote:
> On 2021-01-01 07:29 +0800, Xi Ruoyao wrote:
>> An invalid use of MSA_SUPPORTED_MODE_P is causing ICE on mips64el with -mmsa.
>> The detailed analysis is posted on bugzilla:
>>
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98491
>>
>> The attached patch fixes this issue by handling the special case of
>> MSA_SUPPORTED_MODE_P explicitly.
>>
>> Please keep me in CC since I'm not a subscriber.
>>
>> And, I don't have GIT write access.
> Sorry, I forgot to include the ChangeLog:
>
>     gcc/ChangeLog:
>     
>     2020-12-31  Xi Ruoyao <xry111@mengyan1223.wang>
>     
>             PR target/98491
>             * config/mips/mips.c (mips_symbol_insns): Do not use
>               MSA_SUPPORTED_MODE_P if mode is MAX_MACHINE_MODE.
So I absolutely agree the current code is wrong as it does an out of
bounds array access.


Would it be better to instead to change MSA_SUPPORTED_MODE_P to evaluate
to zero if MODE is MAX_MACHINE_MODE?  That would protect all the uses of
MSA_SUPPORTED_MODE_P.    Something like this perhaps?



Jeff

[-- Attachment #2: P --]
[-- Type: text/plain, Size: 522 bytes --]

diff --git a/gcc/config/mips/mips.h b/gcc/config/mips/mips.h
index b4a60a55d80..a159bb22381 100644
--- a/gcc/config/mips/mips.h
+++ b/gcc/config/mips/mips.h
@@ -2418,6 +2418,7 @@ enum reg_class
 /* True if MODE is vector and supported in a MSA vector register.  */
 #define MSA_SUPPORTED_MODE_P(MODE)			\
   (ISA_HAS_MSA						\
+   && (MODE) != MAX_MACHINE_MODE
    && GET_MODE_SIZE (MODE) == UNITS_PER_MSA_REG		\
    && (GET_MODE_CLASS (MODE) == MODE_VECTOR_INT		\
        || GET_MODE_CLASS (MODE) == MODE_VECTOR_FLOAT))

Re: [PATCH] MIPS: Fix PR target/98491 (ChangeLog)

[Jakub Jelinek]

On Mon, Jan 04, 2021 at 01:51:59PM -0700, Jeff Law via Gcc-patches wrote:
> > Sorry, I forgot to include the ChangeLog:
> >
> >     gcc/ChangeLog:
> >     
> >     2020-12-31  Xi Ruoyao <xry111@mengyan1223.wang>
> >     
> >             PR target/98491
> >             * config/mips/mips.c (mips_symbol_insns): Do not use
> >               MSA_SUPPORTED_MODE_P if mode is MAX_MACHINE_MODE.
> So I absolutely agree the current code is wrong as it does an out of
> bounds array access.
> 
> 
> Would it be better to instead to change MSA_SUPPORTED_MODE_P to evaluate
> to zero if MODE is MAX_MACHINE_MODE?  That would protect all the uses of
> MSA_SUPPORTED_MODE_P.    Something like this perhaps?

But MAX_MACHINE_MODE is the one past last valid mode, I'm not aware of
any target that would protect all macros that deal with modes that way.

So, perhaps best would be stop using the MAX_MACHINE_MODE as magic value
for that function and instead use say VOIDmode that shouldn't normally
appear either?

But I don't really see anything wrong on the mips_symbol_insns above
change either.

	Jakub

Re: [PATCH] MIPS: Fix PR target/98491 (ChangeLog)

[Jeff Law]

On 1/4/21 2:00 PM, Jakub Jelinek wrote:
> On Mon, Jan 04, 2021 at 01:51:59PM -0700, Jeff Law via Gcc-patches wrote:
>>> Sorry, I forgot to include the ChangeLog:
>>>
>>>     gcc/ChangeLog:
>>>     
>>>     2020-12-31  Xi Ruoyao <xry111@mengyan1223.wang>
>>>     
>>>             PR target/98491
>>>             * config/mips/mips.c (mips_symbol_insns): Do not use
>>>               MSA_SUPPORTED_MODE_P if mode is MAX_MACHINE_MODE.
>> So I absolutely agree the current code is wrong as it does an out of
>> bounds array access.
>>
>>
>> Would it be better to instead to change MSA_SUPPORTED_MODE_P to evaluate
>> to zero if MODE is MAX_MACHINE_MODE?  That would protect all the uses of
>> MSA_SUPPORTED_MODE_P.    Something like this perhaps?
> But MAX_MACHINE_MODE is the one past last valid mode, I'm not aware of
> any target that would protect all macros that deal with modes that way.
>
> So, perhaps best would be stop using the MAX_MACHINE_MODE as magic value
> for that function and instead use say VOIDmode that shouldn't normally
> appear either?
I think we have to allow VOIDmode because constants don't necessarily
have modes.   And I certainly agree that using MAX_MACHINE_MODE like
this is ugly and error prone (as we can see from the BZ).

I also couldn't convince myself that the code and comments were actually
consistent, particularly for MSA targets which the comment claims can
never handle constants for ld/st (and thus should be returning 0 for
MAX_MACHINE_MODE).  Though maybe mips_symbol_insns_1 ultimately handles
that correctly.


>
> But I don't really see anything wrong on the mips_symbol_insns above
> change either.
Me neither.  I'm just questioning if bullet-proofing in the
MSA_SUPPORTED_MODE_P would be a better option.  While I've worked in the
MIPS port in the past, I don't really have any significannt experience
with the MSA support.

jeff

Re: [PATCH] MIPS: Fix PR target/98491 (ChangeLog)

[Xi Ruoyao]

Hi Jeff and Jakub,

On 2021-01-04 14:19 -0700, Jeff Law wrote:
> On 1/4/21 2:00 PM, Jakub Jelinek wrote:
> > On Mon, Jan 04, 2021 at 01:51:59PM -0700, Jeff Law via Gcc-patches wrote:
> > > > Sorry, I forgot to include the ChangeLog:
> > > > 
> > > >     gcc/ChangeLog:
> > > >     
> > > >     2020-12-31  Xi Ruoyao <xry111@mengyan1223.wang>
> > > >     
> > > >             PR target/98491
> > > >             * config/mips/mips.c (mips_symbol_insns): Do not use
> > > >               MSA_SUPPORTED_MODE_P if mode is MAX_MACHINE_MODE.
> > > So I absolutely agree the current code is wrong as it does an out of
> > > bounds array access.
> > > 
> > > 
> > > Would it be better to instead to change MSA_SUPPORTED_MODE_P to evaluate
> > > to zero if MODE is MAX_MACHINE_MODE?  That would protect all the uses of
> > > MSA_SUPPORTED_MODE_P.    Something like this perhaps?
> > But MAX_MACHINE_MODE is the one past last valid mode, I'm not aware of
> > any target that would protect all macros that deal with modes that way.
> > 
> > So, perhaps best would be stop using the MAX_MACHINE_MODE as magic value
> > for that function and instead use say VOIDmode that shouldn't normally
> > appear either?
> I think we have to allow VOIDmode because constants don't necessarily
> have modes.   And I certainly agree that using MAX_MACHINE_MODE like
> this is ugly and error prone (as we can see from the BZ).
> 
> I also couldn't convince myself that the code and comments were actually
> consistent, particularly for MSA targets which the comment claims can
> never handle constants for ld/st (and thus should be returning 0 for
> MAX_MACHINE_MODE).  Though maybe mips_symbol_insns_1 ultimately handles
> that correctly.
> 
> 
> > 
> > But I don't really see anything wrong on the mips_symbol_insns above
> > change either.
> Me neither.  I'm just questioning if bullet-proofing in the
> MSA_SUPPORTED_MODE_P would be a better option.  While I've worked in the
> MIPS port in the past, I don't really have any significannt experience
> with the MSA support.

I can't understand the comment either.  To me it looks like it's possible to
remove this "if (MSA_SUPPORTED_P (mode)) return 0;"

CC Robert to get some help.
-- 
Xi Ruoyao <xry111@mengyan1223.wang>
School of Aerospace Science and Technology, Xidian University

Re: [PATCH] MIPS: Fix PR target/98491 (ChangeLog)

[Xi Ruoyao]

On 2021-01-11 01:01 +0800, Xi Ruoyao wrote:
> CC Robert to get some help.

Unfortunately Robert's mail in MAINTAINER file seems no longer valid :(.
-- 
Xi Ruoyao <xry111@mengyan1223.wang>
School of Aerospace Science and Technology, Xidian University

Re: [PATCH] MIPS: Fix PR target/98491 (ChangeLog)

[Xi Ruoyao]

On 2021-01-11 01:01 +0800, Xi Ruoyao wrote:
> Hi Jeff and Jakub,
> 
> On 2021-01-04 14:19 -0700, Jeff Law wrote:
> > On 1/4/21 2:00 PM, Jakub Jelinek wrote:
> > > On Mon, Jan 04, 2021 at 01:51:59PM -0700, Jeff Law via Gcc-patches wrote:
> > > > > Sorry, I forgot to include the ChangeLog:
> > > > > 
> > > > >     gcc/ChangeLog:
> > > > >     
> > > > >     2020-12-31  Xi Ruoyao <xry111@mengyan1223.wang>
> > > > >     
> > > > >             PR target/98491
> > > > >             * config/mips/mips.c (mips_symbol_insns): Do not use
> > > > >               MSA_SUPPORTED_MODE_P if mode is MAX_MACHINE_MODE.
> > > > So I absolutely agree the current code is wrong as it does an out of
> > > > bounds array access.
> > > > 
> > > > 
> > > > Would it be better to instead to change MSA_SUPPORTED_MODE_P to evaluate
> > > > to zero if MODE is MAX_MACHINE_MODE?  That would protect all the uses of
> > > > MSA_SUPPORTED_MODE_P.    Something like this perhaps?
> > > But MAX_MACHINE_MODE is the one past last valid mode, I'm not aware of
> > > any target that would protect all macros that deal with modes that way.
> > > 
> > > So, perhaps best would be stop using the MAX_MACHINE_MODE as magic value
> > > for that function and instead use say VOIDmode that shouldn't normally
> > > appear either?
> > I think we have to allow VOIDmode because constants don't necessarily
> > have modes.   And I certainly agree that using MAX_MACHINE_MODE like
> > this is ugly and error prone (as we can see from the BZ).
> > 
> > I also couldn't convince myself that the code and comments were actually
> > consistent, particularly for MSA targets which the comment claims can
> > never handle constants for ld/st (and thus should be returning 0 for
> > MAX_MACHINE_MODE).  Though maybe mips_symbol_insns_1 ultimately handles
> > that correctly.
> > 
> > 
> > > 
> > > But I don't really see anything wrong on the mips_symbol_insns above
> > > change either.
> > Me neither.  I'm just questioning if bullet-proofing in the
> > MSA_SUPPORTED_MODE_P would be a better option.  While I've worked in the
> > MIPS port in the past, I don't really have any significannt experience
> > with the MSA support.
> 
> I can't understand the comment either.  To me it looks like it's possible to
> remove this "if (MSA_SUPPORTED_P (mode)) return 0;"
> 
> CC Robert to get some help.

Happy new lunar year folks.

I found a newer email address of Robert.  Hope it is still being used.

Could someone update MAINTAINERS file by the way?
-- 
Xi Ruoyao <xry111@mengyan1223.wang>
School of Aerospace Science and Technology, Xidian University

Re: [PATCH] MIPS: Fix PR target/98491 (ChangeLog)

[Xi Ruoyao]

Resend the mail.  I had to fill in a form to send mail to Robert.

On 2021-02-12 22:17 +0800, Xi Ruoyao wrote:
> On 2021-01-11 01:01 +0800, Xi Ruoyao wrote:
> > Hi Jeff and Jakub,
> > 
> > On 2021-01-04 14:19 -0700, Jeff Law wrote:
> > > On 1/4/21 2:00 PM, Jakub Jelinek wrote:
> > > > On Mon, Jan 04, 2021 at 01:51:59PM -0700, Jeff Law via Gcc-patches
> > > > wrote:
> > > > > > Sorry, I forgot to include the ChangeLog:
> > > > > > 
> > > > > >     gcc/ChangeLog:
> > > > > >     
> > > > > >     2020-12-31  Xi Ruoyao <xry111@mengyan1223.wang>
> > > > > >     
> > > > > >             PR target/98491
> > > > > >             * config/mips/mips.c (mips_symbol_insns): Do not use
> > > > > >               MSA_SUPPORTED_MODE_P if mode is MAX_MACHINE_MODE.
> > > > > So I absolutely agree the current code is wrong as it does an out of
> > > > > bounds array access.
> > > > > 
> > > > > 
> > > > > Would it be better to instead to change MSA_SUPPORTED_MODE_P to
> > > > > evaluate
> > > > > to zero if MODE is MAX_MACHINE_MODE?  That would protect all the uses
> > > > > of
> > > > > MSA_SUPPORTED_MODE_P.    Something like this perhaps?
> > > > But MAX_MACHINE_MODE is the one past last valid mode, I'm not aware of
> > > > any target that would protect all macros that deal with modes that way.
> > > > 
> > > > So, perhaps best would be stop using the MAX_MACHINE_MODE as magic value
> > > > for that function and instead use say VOIDmode that shouldn't normally
> > > > appear either?
> > > I think we have to allow VOIDmode because constants don't necessarily
> > > have modes.   And I certainly agree that using MAX_MACHINE_MODE like
> > > this is ugly and error prone (as we can see from the BZ).
> > > 
> > > I also couldn't convince myself that the code and comments were actually
> > > consistent, particularly for MSA targets which the comment claims can
> > > never handle constants for ld/st (and thus should be returning 0 for
> > > MAX_MACHINE_MODE).  Though maybe mips_symbol_insns_1 ultimately handles
> > > that correctly.
> > > 
> > > 
> > > > 
> > > > But I don't really see anything wrong on the mips_symbol_insns above
> > > > change either.
> > > Me neither.  I'm just questioning if bullet-proofing in the
> > > MSA_SUPPORTED_MODE_P would be a better option.  While I've worked in the
> > > MIPS port in the past, I don't really have any significannt experience
> > > with the MSA support.
> > 
> > I can't understand the comment either.  To me it looks like it's possible to
> > remove this "if (MSA_SUPPORTED_P (mode)) return 0;"
> > 
> > CC Robert to get some help.
> 
> Happy new lunar year folks.
> 
> I found a newer email address of Robert.  Hope it is still being used.
> 
> Could someone update MAINTAINERS file by the way?

-- 
Xi Ruoyao <xry111@mengyan1223.wang>
School of Aerospace Science and Technology, Xidian University

Re: [PATCH] MIPS: Fix PR target/98491 (ChangeLog)

[Xi Ruoyao]

Well, it just dislike my mail server :(.  Switch to the mail server of my
university.

On 2021-02-12 22:54 +0800, Xi Ruoyao wrote:
> Resend the mail.  I had to fill in a form to send mail to Robert.
> 
> On 2021-02-12 22:17 +0800, Xi Ruoyao wrote:
> > On 2021-01-11 01:01 +0800, Xi Ruoyao wrote:
> > > Hi Jeff and Jakub,
> > > 
> > > On 2021-01-04 14:19 -0700, Jeff Law wrote:
> > > > On 1/4/21 2:00 PM, Jakub Jelinek wrote:
> > > > > On Mon, Jan 04, 2021 at 01:51:59PM -0700, Jeff Law via Gcc-patches
> > > > > wrote:
> > > > > > > Sorry, I forgot to include the ChangeLog:
> > > > > > > 
> > > > > > >     gcc/ChangeLog:
> > > > > > >     
> > > > > > >     2020-12-31  Xi Ruoyao <xry111@mengyan1223.wang>
> > > > > > >     
> > > > > > >             PR target/98491
> > > > > > >             * config/mips/mips.c (mips_symbol_insns): Do not use
> > > > > > >               MSA_SUPPORTED_MODE_P if mode is MAX_MACHINE_MODE.
> > > > > > So I absolutely agree the current code is wrong as it does an out of
> > > > > > bounds array access.
> > > > > > 
> > > > > > 
> > > > > > Would it be better to instead to change MSA_SUPPORTED_MODE_P to
> > > > > > evaluate
> > > > > > to zero if MODE is MAX_MACHINE_MODE?  That would protect all the
> > > > > > uses
> > > > > > of
> > > > > > MSA_SUPPORTED_MODE_P.    Something like this perhaps?
> > > > > But MAX_MACHINE_MODE is the one past last valid mode, I'm not aware of
> > > > > any target that would protect all macros that deal with modes that
> > > > > way.
> > > > > 
> > > > > So, perhaps best would be stop using the MAX_MACHINE_MODE as magic
> > > > > value
> > > > > for that function and instead use say VOIDmode that shouldn't normally
> > > > > appear either?
> > > > I think we have to allow VOIDmode because constants don't necessarily
> > > > have modes.   And I certainly agree that using MAX_MACHINE_MODE like
> > > > this is ugly and error prone (as we can see from the BZ).
> > > > 
> > > > I also couldn't convince myself that the code and comments were actually
> > > > consistent, particularly for MSA targets which the comment claims can
> > > > never handle constants for ld/st (and thus should be returning 0 for
> > > > MAX_MACHINE_MODE).  Though maybe mips_symbol_insns_1 ultimately handles
> > > > that correctly.
> > > > 
> > > > 
> > > > > 
> > > > > But I don't really see anything wrong on the mips_symbol_insns above
> > > > > change either.
> > > > Me neither.  I'm just questioning if bullet-proofing in the
> > > > MSA_SUPPORTED_MODE_P would be a better option.  While I've worked in the
> > > > MIPS port in the past, I don't really have any significannt experience
> > > > with the MSA support.
> > > 
> > > I can't understand the comment either.  To me it looks like it's possible
> > > to
> > > remove this "if (MSA_SUPPORTED_P (mode)) return 0;"
> > > 
> > > CC Robert to get some help.
> > 
> > Happy new lunar year folks.
> > 
> > I found a newer email address of Robert.  Hope it is still being used.
> > 
> > Could someone update MAINTAINERS file by the way?
> 

Re: [PATCH] MIPS: Fix PR target/98491 (ChangeLog)

[Xi Ruoyao]

Nope.  I can't reach Robert, so CC MIPS maintainer.

On 2021-02-12 22:57 +0800，Xi Ruoyao wrote:
> Well, it just dislike my mail server :(.  Switch to the mail server of my
> university.
> 
> On 2021-02-12 22:54 +0800, Xi Ruoyao wrote:
> > Resend the mail.  I had to fill in a form to send mail to Robert.
> > 
> > On 2021-02-12 22:17 +0800, Xi Ruoyao wrote:
> > > On 2021-01-11 01:01 +0800, Xi Ruoyao wrote:
> > > > Hi Jeff and Jakub,
> > > > 
> > > > On 2021-01-04 14:19 -0700, Jeff Law wrote:
> > > > > On 1/4/21 2:00 PM, Jakub Jelinek wrote:
> > > > > > On Mon, Jan 04, 2021 at 01:51:59PM -0700, Jeff Law via Gcc-patches
> > > > > > wrote:
> > > > > > > > Sorry, I forgot to include the ChangeLog:
> > > > > > > > 
> > > > > > > >     gcc/ChangeLog:
> > > > > > > >     
> > > > > > > >     2020-12-31  Xi Ruoyao <xry111@mengyan1223.wang>
> > > > > > > >     
> > > > > > > >             PR target/98491
> > > > > > > >             * config/mips/mips.c (mips_symbol_insns): Do not use
> > > > > > > >               MSA_SUPPORTED_MODE_P if mode is MAX_MACHINE_MODE.
> > > > > > > So I absolutely agree the current code is wrong as it does an out
> > > > > > > of
> > > > > > > bounds array access.
> > > > > > > 
> > > > > > > 
> > > > > > > Would it be better to instead to change MSA_SUPPORTED_MODE_P to
> > > > > > > evaluate
> > > > > > > to zero if MODE is MAX_MACHINE_MODE?  That would protect all the
> > > > > > > uses
> > > > > > > of
> > > > > > > MSA_SUPPORTED_MODE_P.    Something like this perhaps?
> > > > > > But MAX_MACHINE_MODE is the one past last valid mode, I'm not aware
> > > > > > of
> > > > > > any target that would protect all macros that deal with modes that
> > > > > > way.
> > > > > > 
> > > > > > So, perhaps best would be stop using the MAX_MACHINE_MODE as magic
> > > > > > value
> > > > > > for that function and instead use say VOIDmode that shouldn't
> > > > > > normally
> > > > > > appear either?
> > > > > I think we have to allow VOIDmode because constants don't necessarily
> > > > > have modes.   And I certainly agree that using MAX_MACHINE_MODE like
> > > > > this is ugly and error prone (as we can see from the BZ).
> > > > > 
> > > > > I also couldn't convince myself that the code and comments were
> > > > > actually
> > > > > consistent, particularly for MSA targets which the comment claims can
> > > > > never handle constants for ld/st (and thus should be returning 0 for
> > > > > MAX_MACHINE_MODE).  Though maybe mips_symbol_insns_1 ultimately
> > > > > handles
> > > > > that correctly.
> > > > > 
> > > > > 
> > > > > > 
> > > > > > But I don't really see anything wrong on the mips_symbol_insns above
> > > > > > change either.
> > > > > Me neither.  I'm just questioning if bullet-proofing in the
> > > > > MSA_SUPPORTED_MODE_P would be a better option.  While I've worked in
> > > > > the
> > > > > MIPS port in the past, I don't really have any significannt experience
> > > > > with the MSA support.
> > > > 
> > > > I can't understand the comment either.  To me it looks like it's
> > > > possible
> > > > to
> > > > remove this "if (MSA_SUPPORTED_P (mode)) return 0;"
> > > > 
> > > > CC Robert to get some help.
> > > 
> > > Happy new lunar year folks.
> > > 
> > > I found a newer email address of Robert.  Hope it is still being used.
> > > 
> > > Could someone update MAINTAINERS file by the way?
> > 
> 
> 

-- 
Xi Ruoyao <xry111@mengyan1223.wang>
School of Aerospace Science and Technology, Xidian University

Re: [PATCH] MIPS: Fix PR target/98491 (ChangeLog)

[Jeff Law]

On 2/12/21 7:17 AM, Xi Ruoyao wrote:
> On 2021-01-11 01:01 +0800, Xi Ruoyao wrote:
>> Hi Jeff and Jakub,
>>
>> On 2021-01-04 14:19 -0700, Jeff Law wrote:
>>> On 1/4/21 2:00 PM, Jakub Jelinek wrote:
>>>> On Mon, Jan 04, 2021 at 01:51:59PM -0700, Jeff Law via Gcc-patches wrote:
>>>>>> Sorry, I forgot to include the ChangeLog:
>>>>>>
>>>>>>     gcc/ChangeLog:
>>>>>>     
>>>>>>     2020-12-31  Xi Ruoyao <xry111@mengyan1223.wang>
>>>>>>     
>>>>>>             PR target/98491
>>>>>>             * config/mips/mips.c (mips_symbol_insns): Do not use
>>>>>>               MSA_SUPPORTED_MODE_P if mode is MAX_MACHINE_MODE.
>>>>> So I absolutely agree the current code is wrong as it does an out of
>>>>> bounds array access.
>>>>>
>>>>>
>>>>> Would it be better to instead to change MSA_SUPPORTED_MODE_P to evaluate
>>>>> to zero if MODE is MAX_MACHINE_MODE?  That would protect all the uses of
>>>>> MSA_SUPPORTED_MODE_P.    Something like this perhaps?
>>>> But MAX_MACHINE_MODE is the one past last valid mode, I'm not aware of
>>>> any target that would protect all macros that deal with modes that way.
>>>>
>>>> So, perhaps best would be stop using the MAX_MACHINE_MODE as magic value
>>>> for that function and instead use say VOIDmode that shouldn't normally
>>>> appear either?
>>> I think we have to allow VOIDmode because constants don't necessarily
>>> have modes.   And I certainly agree that using MAX_MACHINE_MODE like
>>> this is ugly and error prone (as we can see from the BZ).
>>>
>>> I also couldn't convince myself that the code and comments were actually
>>> consistent, particularly for MSA targets which the comment claims can
>>> never handle constants for ld/st (and thus should be returning 0 for
>>> MAX_MACHINE_MODE).  Though maybe mips_symbol_insns_1 ultimately handles
>>> that correctly.
>>>
>>>
>>>> But I don't really see anything wrong on the mips_symbol_insns above
>>>> change either.
>>> Me neither.  I'm just questioning if bullet-proofing in the
>>> MSA_SUPPORTED_MODE_P would be a better option.  While I've worked in the
>>> MIPS port in the past, I don't really have any significannt experience
>>> with the MSA support.
>> I can't understand the comment either.  To me it looks like it's possible to
>> remove this "if (MSA_SUPPORTED_P (mode)) return 0;"
>>
>> CC Robert to get some help.
> Happy new lunar year folks.
>
> I found a newer email address of Robert.  Hope it is still being used.
>
> Could someone update MAINTAINERS file by the way?
If you have an updated email address, I can reach out to Robert and see
if he wants his entry updated or removed.

Thanks,
jeff

Re: [PATCH] MIPS: Fix PR target/98491 (ChangeLog)

[Xi Ruoyao]

On 2021-02-15 16:16 -0700, Jeff Law wrote:
> 
> 
> On 2/12/21 7:17 AM, Xi Ruoyao wrote:
> > On 2021-01-11 01:01 +0800, Xi Ruoyao wrote:
> > > Hi Jeff and Jakub,
> > > 
> > > On 2021-01-04 14:19 -0700, Jeff Law wrote:
> > > > On 1/4/21 2:00 PM, Jakub Jelinek wrote:
> > > > > On Mon, Jan 04, 2021 at 01:51:59PM -0700, Jeff Law via Gcc-patches
> > > > > wrote:
> > > > > > > Sorry, I forgot to include the ChangeLog:
> > > > > > > 
> > > > > > >     gcc/ChangeLog:
> > > > > > >     
> > > > > > >     2020-12-31  Xi Ruoyao <xry111@mengyan1223.wang>
> > > > > > >     
> > > > > > >             PR target/98491
> > > > > > >             * config/mips/mips.c (mips_symbol_insns): Do not use
> > > > > > >               MSA_SUPPORTED_MODE_P if mode is MAX_MACHINE_MODE.
> > > > > > So I absolutely agree the current code is wrong as it does an out of
> > > > > > bounds array access.
> > > > > > 
> > > > > > 
> > > > > > Would it be better to instead to change MSA_SUPPORTED_MODE_P to
> > > > > > evaluate
> > > > > > to zero if MODE is MAX_MACHINE_MODE?  That would protect all the uses
> > > > > > of
> > > > > > MSA_SUPPORTED_MODE_P.    Something like this perhaps?
> > > > > But MAX_MACHINE_MODE is the one past last valid mode, I'm not aware of
> > > > > any target that would protect all macros that deal with modes that way.
> > > > > 
> > > > > So, perhaps best would be stop using the MAX_MACHINE_MODE as magic value
> > > > > for that function and instead use say VOIDmode that shouldn't normally
> > > > > appear either?
> > > > I think we have to allow VOIDmode because constants don't necessarily
> > > > have modes.   And I certainly agree that using MAX_MACHINE_MODE like
> > > > this is ugly and error prone (as we can see from the BZ).
> > > > 
> > > > I also couldn't convince myself that the code and comments were actually
> > > > consistent, particularly for MSA targets which the comment claims can
> > > > never handle constants for ld/st (and thus should be returning 0 for
> > > > MAX_MACHINE_MODE).  Though maybe mips_symbol_insns_1 ultimately handles
> > > > that correctly.
> > > > 
> > > > 
> > > > > But I don't really see anything wrong on the mips_symbol_insns above
> > > > > change either.
> > > > Me neither.  I'm just questioning if bullet-proofing in the
> > > > MSA_SUPPORTED_MODE_P would be a better option.  While I've worked in the
> > > > MIPS port in the past, I don't really have any significannt experience
> > > > with the MSA support.
> > > I can't understand the comment either.  To me it looks like it's possible to
> > > remove this "if (MSA_SUPPORTED_P (mode)) return 0;"
> > > 
> > > CC Robert to get some help.
> > Happy new lunar year folks.
> > 
> > I found a newer email address of Robert.  Hope it is still being used.
> > 
> > Could someone update MAINTAINERS file by the way?
> If you have an updated email address, I can reach out to Robert and see
> if he wants his entry updated or removed.

 His latest reply in gcc mail lists used Robert.Suchanek@mips.com.  But when I
sent mail to it, the mail was just rejected with "access denied".  Google told
me Office 365 mail service (used by mips.com) rejects mail to deleted accounts
with "access denied". So I'm not sure if this email address is invalid again, or
Office 365 just dislikes me...
-- 
Xi Ruoyao <xry111@mengyan1223.wang>
School of Aerospace Science and Technology, Xidian University

Re: [PATCH] MIPS: Fix PR target/98491 (ChangeLog)

[Xi Ruoyao]

On 2021-02-16 11:59 +0800, Xi Ruoyao wrote:
> On 2021-02-15 16:16 -0700, Jeff Law wrote:
> > 
> > 
> > On 2/12/21 7:17 AM, Xi Ruoyao wrote:
> > > On 2021-01-11 01:01 +0800, Xi Ruoyao wrote:
> > > > Hi Jeff and Jakub,
> > > > 
> > > > On 2021-01-04 14:19 -0700, Jeff Law wrote:
> > > > > On 1/4/21 2:00 PM, Jakub Jelinek wrote:
> > > > > > On Mon, Jan 04, 2021 at 01:51:59PM -0700, Jeff Law via Gcc-patches
> > > > > > wrote:
> > > > > > > > Sorry, I forgot to include the ChangeLog:
> > > > > > > > 
> > > > > > > >     gcc/ChangeLog:
> > > > > > > >     
> > > > > > > >     2020-12-31  Xi Ruoyao <xry111@mengyan1223.wang>
> > > > > > > >     
> > > > > > > >             PR target/98491
> > > > > > > >             * config/mips/mips.c (mips_symbol_insns): Do not use
> > > > > > > >               MSA_SUPPORTED_MODE_P if mode is MAX_MACHINE_MODE.
> > > > > > > So I absolutely agree the current code is wrong as it does an out
> > > > > > > of
> > > > > > > bounds array access.
> > > > > > > 
> > > > > > > 
> > > > > > > Would it be better to instead to change MSA_SUPPORTED_MODE_P to
> > > > > > > evaluate
> > > > > > > to zero if MODE is MAX_MACHINE_MODE?  That would protect all the
> > > > > > > uses
> > > > > > > of
> > > > > > > MSA_SUPPORTED_MODE_P.    Something like this perhaps?
> > > > > > But MAX_MACHINE_MODE is the one past last valid mode, I'm not aware
> > > > > > of
> > > > > > any target that would protect all macros that deal with modes that
> > > > > > way.
> > > > > > 
> > > > > > So, perhaps best would be stop using the MAX_MACHINE_MODE as magic
> > > > > > value
> > > > > > for that function and instead use say VOIDmode that shouldn't
> > > > > > normally
> > > > > > appear either?
> > > > > I think we have to allow VOIDmode because constants don't necessarily
> > > > > have modes.   And I certainly agree that using MAX_MACHINE_MODE like
> > > > > this is ugly and error prone (as we can see from the BZ).
> > > > > 
> > > > > I also couldn't convince myself that the code and comments were
> > > > > actually
> > > > > consistent, particularly for MSA targets which the comment claims can
> > > > > never handle constants for ld/st (and thus should be returning 0 for
> > > > > MAX_MACHINE_MODE).  Though maybe mips_symbol_insns_1 ultimately
> > > > > handles
> > > > > that correctly.
> > > > > 
> > > > > 
> > > > > > But I don't really see anything wrong on the mips_symbol_insns above
> > > > > > change either.
> > > > > Me neither.  I'm just questioning if bullet-proofing in the
> > > > > MSA_SUPPORTED_MODE_P would be a better option.  While I've worked in
> > > > > the
> > > > > MIPS port in the past, I don't really have any significannt experience
> > > > > with the MSA support.
> > > > I can't understand the comment either.  To me it looks like it's
> > > > possible to
> > > > remove this "if (MSA_SUPPORTED_P (mode)) return 0;"
> > > > 
> > > > CC Robert to get some help.
> > > Happy new lunar year folks.
> > > 
> > > I found a newer email address of Robert.  Hope it is still being used.
> > > 
> > > Could someone update MAINTAINERS file by the way?
> > If you have an updated email address, I can reach out to Robert and see
> > if he wants his entry updated or removed.
> 
>  His latest reply in gcc mail lists used Robert.Suchanek@mips.com.  But when I
> sent mail to it, the mail was just rejected with "access denied".  Google told
> me Office 365 mail service (used by mips.com) rejects mail to deleted accounts
> with "access denied". So I'm not sure if this email address is invalid again,
> or
> Office 365 just dislikes me...

Hi Jeff,

I think it's better to just fix the out-of-bound array access now by special
casing MAX_MACHINE_MODE, if we can't figure out if this entry should be removed.
Either in MSA_SUPPORTED_P or in mips_symbol_insns.

It's really "irrational" to leave such a obvious programming error in new GCC 11
release...  And I've built a Linux system (in Linux From Scratch way, X11 was
built and it runs correctly now) on the Loongson 3A4000 with patched GCC-10.2.0,
and "-O3 -mmsa" in CFLAGS for most packages so the change should be OK.
-- 
Xi Ruoyao <xry111@mengyan1223.wang>
School of Aerospace Science and Technology, Xidian University

Re: [PATCH] MIPS: Fix PR target/98491 (ChangeLog)

[Richard Sandiford]

Xi Ruoyao via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
>> > > > I can't understand the comment either.  To me it looks like it's
>> > > > possible to
>> > > > remove this "if (MSA_SUPPORTED_P (mode)) return 0;"

I think the point is that the MSA loads and stores only have a 10-bit
offset field instead of the usual 16-bit offset field and so the usual
approaches to handling symbolic addresses won't work.

>> > > > 
>> > > > CC Robert to get some help.
>> > > Happy new lunar year folks.
>> > > 
>> > > I found a newer email address of Robert.  Hope it is still being used.
>> > > 
>> > > Could someone update MAINTAINERS file by the way?
>> > If you have an updated email address, I can reach out to Robert and see
>> > if he wants his entry updated or removed.
>> 
>>  His latest reply in gcc mail lists used Robert.Suchanek@mips.com.  But when I
>> sent mail to it, the mail was just rejected with "access denied".  Google told
>> me Office 365 mail service (used by mips.com) rejects mail to deleted accounts
>> with "access denied". So I'm not sure if this email address is invalid again,
>> or
>> Office 365 just dislikes me...
>
> Hi Jeff,
>
> I think it's better to just fix the out-of-bound array access now by special
> casing MAX_MACHINE_MODE, if we can't figure out if this entry should be removed.
> Either in MSA_SUPPORTED_P or in mips_symbol_insns.
>
> It's really "irrational" to leave such a obvious programming error in new GCC 11
> release...  And I've built a Linux system (in Linux From Scratch way, X11 was
> built and it runs correctly now) on the Loongson 3A4000 with patched GCC-10.2.0,
> and "-O3 -mmsa" in CFLAGS for most packages so the change should be OK.

Yeah, agreed.  I think the mips_symbol_insns patch is the right one,
so I've pushed to it trunk.  I think it's also worth backporting
to release branches, but let me know how far back you've tested it.

Thanks for the patch.

Richard

Re: [PATCH] MIPS: Fix PR target/98491 (ChangeLog)

[Jeff Law]

On 2/17/21 5:02 AM, Richard Sandiford wrote:
> Xi Ruoyao via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
>>>>>> I can't understand the comment either.  To me it looks like it's
>>>>>> possible to
>>>>>> remove this "if (MSA_SUPPORTED_P (mode)) return 0;"
> I think the point is that the MSA loads and stores only have a 10-bit
> offset field instead of the usual 16-bit offset field and so the usual
> approaches to handling symbolic addresses won't work.
Ah.

>
>>>>>> CC Robert to get some help.
>>>>> Happy new lunar year folks.
>>>>>
>>>>> I found a newer email address of Robert.  Hope it is still being used.
>>>>>
>>>>> Could someone update MAINTAINERS file by the way?
>>>> If you have an updated email address, I can reach out to Robert and see
>>>> if he wants his entry updated or removed.
>>>  His latest reply in gcc mail lists used Robert.Suchanek@mips.com.  But when I
>>> sent mail to it, the mail was just rejected with "access denied".  Google told
>>> me Office 365 mail service (used by mips.com) rejects mail to deleted accounts
>>> with "access denied". So I'm not sure if this email address is invalid again,
>>> or
>>> Office 365 just dislikes me...
>> Hi Jeff,
>>
>> I think it's better to just fix the out-of-bound array access now by special
>> casing MAX_MACHINE_MODE, if we can't figure out if this entry should be removed.
>> Either in MSA_SUPPORTED_P or in mips_symbol_insns.
>>
>> It's really "irrational" to leave such a obvious programming error in new GCC 11
>> release...  And I've built a Linux system (in Linux From Scratch way, X11 was
>> built and it runs correctly now) on the Loongson 3A4000 with patched GCC-10.2.0,
>> and "-O3 -mmsa" in CFLAGS for most packages so the change should be OK.
> Yeah, agreed.  I think the mips_symbol_insns patch is the right one,
> so I've pushed to it trunk.  I think it's also worth backporting
> to release branches, but let me know how far back you've tested it.
Seems reasonable to me.  THanks for lending a hand on this Richard.

jeff