* [PATCH] MIPS: Fix PR target/98491 @ 2020-12-31 23:29 Xi Ruoyao 2020-12-31 23:34 ` [PATCH] MIPS: Fix PR target/98491 (ChangeLog) Xi Ruoyao 0 siblings, 1 reply; 16+ messages in thread From: Xi Ruoyao @ 2020-12-31 23:29 UTC (permalink / raw) To: gcc-patches; +Cc: 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); ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] MIPS: Fix PR target/98491 (ChangeLog) 2020-12-31 23:29 [PATCH] MIPS: Fix PR target/98491 Xi Ruoyao @ 2020-12-31 23:34 ` Xi Ruoyao 2021-01-04 20:51 ` Jeff Law 0 siblings, 1 reply; 16+ messages in thread From: Xi Ruoyao @ 2020-12-31 23:34 UTC (permalink / raw) To: gcc-patches 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] MIPS: Fix PR target/98491 (ChangeLog) 2020-12-31 23:34 ` [PATCH] MIPS: Fix PR target/98491 (ChangeLog) Xi Ruoyao @ 2021-01-04 20:51 ` Jeff Law 2021-01-04 21:00 ` Jakub Jelinek 0 siblings, 1 reply; 16+ messages in thread From: Jeff Law @ 2021-01-04 20:51 UTC (permalink / raw) To: Xi Ruoyao, gcc-patches [-- 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)) ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] MIPS: Fix PR target/98491 (ChangeLog) 2021-01-04 20:51 ` Jeff Law @ 2021-01-04 21:00 ` Jakub Jelinek 2021-01-04 21:19 ` Jeff Law 0 siblings, 1 reply; 16+ messages in thread From: Jakub Jelinek @ 2021-01-04 21:00 UTC (permalink / raw) To: Jeff Law; +Cc: Xi Ruoyao, gcc-patches 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] MIPS: Fix PR target/98491 (ChangeLog) 2021-01-04 21:00 ` Jakub Jelinek @ 2021-01-04 21:19 ` Jeff Law 2021-01-10 17:01 ` Xi Ruoyao 0 siblings, 1 reply; 16+ messages in thread From: Jeff Law @ 2021-01-04 21:19 UTC (permalink / raw) To: Jakub Jelinek; +Cc: Xi Ruoyao, gcc-patches 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] MIPS: Fix PR target/98491 (ChangeLog) 2021-01-04 21:19 ` Jeff Law @ 2021-01-10 17:01 ` Xi Ruoyao 2021-01-10 17:04 ` Xi Ruoyao 2021-02-12 14:17 ` Xi Ruoyao 0 siblings, 2 replies; 16+ messages in thread From: Xi Ruoyao @ 2021-01-10 17:01 UTC (permalink / raw) To: Jeff Law, Jakub Jelinek; +Cc: gcc-patches, xry111, Robert Suchanek 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] MIPS: Fix PR target/98491 (ChangeLog) 2021-01-10 17:01 ` Xi Ruoyao @ 2021-01-10 17:04 ` Xi Ruoyao 2021-02-12 14:17 ` Xi Ruoyao 1 sibling, 0 replies; 16+ messages in thread From: Xi Ruoyao @ 2021-01-10 17:04 UTC (permalink / raw) To: Jeff Law, Jakub Jelinek; +Cc: gcc-patches, xry111 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] MIPS: Fix PR target/98491 (ChangeLog) 2021-01-10 17:01 ` Xi Ruoyao 2021-01-10 17:04 ` Xi Ruoyao @ 2021-02-12 14:17 ` Xi Ruoyao 2021-02-12 14:54 ` Xi Ruoyao 2021-02-15 23:16 ` Jeff Law 1 sibling, 2 replies; 16+ messages in thread From: Xi Ruoyao @ 2021-02-12 14:17 UTC (permalink / raw) To: Jeff Law, Jakub Jelinek; +Cc: gcc-patches, Robert Suchanek 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] MIPS: Fix PR target/98491 (ChangeLog) 2021-02-12 14:17 ` Xi Ruoyao @ 2021-02-12 14:54 ` Xi Ruoyao 2021-02-12 14:57 ` Xi Ruoyao 2021-02-15 23:16 ` Jeff Law 1 sibling, 1 reply; 16+ messages in thread From: Xi Ruoyao @ 2021-02-12 14:54 UTC (permalink / raw) To: Jeff Law, Jakub Jelinek; +Cc: gcc-patches, Robert Suchanek 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] MIPS: Fix PR target/98491 (ChangeLog) 2021-02-12 14:54 ` Xi Ruoyao @ 2021-02-12 14:57 ` Xi Ruoyao 2021-02-12 15:15 ` Xi Ruoyao 0 siblings, 1 reply; 16+ messages in thread From: Xi Ruoyao @ 2021-02-12 14:57 UTC (permalink / raw) To: Jeff Law, Jakub Jelinek; +Cc: gcc-patches, Robert Suchanek, xry111 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? > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] MIPS: Fix PR target/98491 (ChangeLog) 2021-02-12 14:57 ` Xi Ruoyao @ 2021-02-12 15:15 ` Xi Ruoyao 0 siblings, 0 replies; 16+ messages in thread From: Xi Ruoyao @ 2021-02-12 15:15 UTC (permalink / raw) To: Jeff Law, Jakub Jelinek; +Cc: gcc-patches, Matthew Fortune 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] MIPS: Fix PR target/98491 (ChangeLog) 2021-02-12 14:17 ` Xi Ruoyao 2021-02-12 14:54 ` Xi Ruoyao @ 2021-02-15 23:16 ` Jeff Law 2021-02-16 3:59 ` Xi Ruoyao 1 sibling, 1 reply; 16+ messages in thread From: Jeff Law @ 2021-02-15 23:16 UTC (permalink / raw) To: Xi Ruoyao, Jakub Jelinek; +Cc: gcc-patches, Robert Suchanek 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] MIPS: Fix PR target/98491 (ChangeLog) 2021-02-15 23:16 ` Jeff Law @ 2021-02-16 3:59 ` Xi Ruoyao 2021-02-17 11:13 ` Xi Ruoyao 0 siblings, 1 reply; 16+ messages in thread From: Xi Ruoyao @ 2021-02-16 3:59 UTC (permalink / raw) To: Jeff Law, Jakub Jelinek; +Cc: gcc-patches 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] MIPS: Fix PR target/98491 (ChangeLog) 2021-02-16 3:59 ` Xi Ruoyao @ 2021-02-17 11:13 ` Xi Ruoyao 2021-02-17 12:02 ` Richard Sandiford 0 siblings, 1 reply; 16+ messages in thread From: Xi Ruoyao @ 2021-02-17 11:13 UTC (permalink / raw) To: Jeff Law, Jakub Jelinek; +Cc: gcc-patches 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] MIPS: Fix PR target/98491 (ChangeLog) 2021-02-17 11:13 ` Xi Ruoyao @ 2021-02-17 12:02 ` Richard Sandiford 2021-03-02 23:16 ` Jeff Law 0 siblings, 1 reply; 16+ messages in thread From: Richard Sandiford @ 2021-02-17 12:02 UTC (permalink / raw) To: Xi Ruoyao via Gcc-patches; +Cc: Jeff Law, Jakub Jelinek, Xi Ruoyao 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] MIPS: Fix PR target/98491 (ChangeLog) 2021-02-17 12:02 ` Richard Sandiford @ 2021-03-02 23:16 ` Jeff Law 0 siblings, 0 replies; 16+ messages in thread From: Jeff Law @ 2021-03-02 23:16 UTC (permalink / raw) To: Xi Ruoyao via Gcc-patches, Jeff Law, Jakub Jelinek, Xi Ruoyao, richard.sandiford 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2021-03-02 23:16 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-12-31 23:29 [PATCH] MIPS: Fix PR target/98491 Xi Ruoyao 2020-12-31 23:34 ` [PATCH] MIPS: Fix PR target/98491 (ChangeLog) Xi Ruoyao 2021-01-04 20:51 ` Jeff Law 2021-01-04 21:00 ` Jakub Jelinek 2021-01-04 21:19 ` Jeff Law 2021-01-10 17:01 ` Xi Ruoyao 2021-01-10 17:04 ` Xi Ruoyao 2021-02-12 14:17 ` Xi Ruoyao 2021-02-12 14:54 ` Xi Ruoyao 2021-02-12 14:57 ` Xi Ruoyao 2021-02-12 15:15 ` Xi Ruoyao 2021-02-15 23:16 ` Jeff Law 2021-02-16 3:59 ` Xi Ruoyao 2021-02-17 11:13 ` Xi Ruoyao 2021-02-17 12:02 ` Richard Sandiford 2021-03-02 23:16 ` Jeff Law
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).