Re: [PATCH] MIPS: Fix PR target/98491 (ChangeLog)
On 2/17/21 5:02 AM, Richard Sandiford wrote: > Xi Ruoyao via Gcc-patches 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.sucha...@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
Re: [PATCH] MIPS: Fix PR target/98491 (ChangeLog)
Xi Ruoyao via Gcc-patches 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.sucha...@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)
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 > > > > > > > > > > > > > > > > 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.sucha...@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 School of Aerospace Science and Technology, Xidian University
Re: [PATCH] MIPS: Fix PR target/98491 (ChangeLog)
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 > > > > > > > > > > > > > > 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.sucha...@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 School of Aerospace Science and Technology, Xidian University
Re: [PATCH] MIPS: Fix PR target/98491 (ChangeLog)
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 >> >> 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)
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 > > > > > > > > > > > > > > > > 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 School of Aerospace Science and Technology, Xidian University
Re: [PATCH] MIPS: Fix PR target/98491 (ChangeLog)
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 > > > > > > > > > > > > > > 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)
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 > > > > > > > > > > > > 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 School of Aerospace Science and Technology, Xidian University
Re: [PATCH] MIPS: Fix PR target/98491 (ChangeLog)
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 > > > > > > > > > > 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 School of Aerospace Science and Technology, Xidian University
Re: [PATCH] MIPS: Fix PR target/98491 (ChangeLog)
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 School of Aerospace Science and Technology, Xidian University
Re: [PATCH] MIPS: Fix PR target/98491 (ChangeLog)
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 > > > > > > > > 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 School of Aerospace Science and Technology, Xidian University
Re: [PATCH] MIPS: Fix PR target/98491 (ChangeLog)
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 >>> >>> 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)
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 > > > > 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)
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 > > 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 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)
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 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 School of Aerospace Science and Technology, Xidian University
