Message ID | 200323ec-7686-44f6-a5de-edc726909141@BAMAIL02.ba.imgtec.org |
---|---|
State | New |
Headers | show |
"Steve Ellcey " <sellcey@imgtec.com> writes: > While testing GCC on a 74k MIPS chip I noticed that by default the -mtune=74k* > flags cause GCC to not use the integer madd/msub instructions. According to > the checkin comments these were found to cause a performance hit over using > individual mult and add/sub instructions. I think there are some programs > though where using madd/msub would be a win on the 74k and I would like to > have a flag to allow users to override the default behaviour (either turning > it on for 74k or turning it off for other achitectures). This patch allows > -mimadd or -mno-imadd to override the default behaviour but does not change > that default behaviour. This is similar in spirit to -mbranch-likely. It'd be good for consistency if they were defined in a similar style. I think that means removing !TARGET_MIPS16 from ISA_HAS_MADD_MSUB and instead having: #define GENERATE_MADD_MSUB (TARGET_IMADD && !TARGET_MIPS16) There would also be: #define PTF_AVOID_IMADD 0x2 which should be included in the 74k description, and a block similar to the MASK_BRANCHLIKELY one in mips_option_override. There needs to be documentation in invoke.texi. But -- sorry for the soapbox speech -- it would be better to retune so that new options aren't needed. I'm assuming you're testing against the same microarchitecture that the original 74k authors were. If so, it seems like -mimadd is just an option for choosing between two bad implementations. One uses MADD and MSUB unconditionally (contrary to the experience of the original authors) and the other never uses it at all (contrary to your experience). That's not enough reason to reject the patch, just saying :-) Thanks, Richard
On Sat, 2013-03-23 at 14:50 +0000, Richard Sandiford wrote: > This is similar in spirit to -mbranch-likely. It'd be good for consistency > if they were defined in a similar style. I think that means removing > !TARGET_MIPS16 from ISA_HAS_MADD_MSUB and instead having: > > #define GENERATE_MADD_MSUB (TARGET_IMADD && !TARGET_MIPS16) > > There would also be: > > #define PTF_AVOID_IMADD 0x2 > > which should be included in the 74k description, and a block similar to > the MASK_BRANCHLIKELY one in mips_option_override. There needs to be > documentation in invoke.texi. I can do it this way if you want, I was using -mllsc as my template for how to implement this. Do you think the -mllsc flag should be implemented in the same way as -mbranch-likely? > But -- sorry for the soapbox speech -- it would be better to retune > so that new options aren't needed. I'm assuming you're testing against > the same microarchitecture that the original 74k authors were. If so, > it seems like -mimadd is just an option for choosing between two bad > implementations. One uses MADD and MSUB unconditionally (contrary to > the experience of the original authors) and the other never uses it > at all (contrary to your experience). > > That's not enough reason to reject the patch, just saying :-) I agree that the 74k should only be using the integer madd/msub instruction where it makes sense but I think having a flag to allow the user to override it is still a good thing because the compiler won't always be right. Actually, one of my reasons for adding this flag is to make it easier for me to do 74k runs with and without madd/msub and see where we are using (but shouldn't) and hopefully improve the current implementation. Steve Ellcey sellcey@mips.com
Steve Ellcey <sellcey@imgtec.com> writes: > On Sat, 2013-03-23 at 14:50 +0000, Richard Sandiford wrote: >> This is similar in spirit to -mbranch-likely. It'd be good for consistency >> if they were defined in a similar style. I think that means removing >> !TARGET_MIPS16 from ISA_HAS_MADD_MSUB and instead having: >> >> #define GENERATE_MADD_MSUB (TARGET_IMADD && !TARGET_MIPS16) >> >> There would also be: >> >> #define PTF_AVOID_IMADD 0x2 >> >> which should be included in the 74k description, and a block similar to >> the MASK_BRANCHLIKELY one in mips_option_override. There needs to be >> documentation in invoke.texi. > > I can do it this way if you want, I was using -mllsc as my template for > how to implement this. Do you think the -mllsc flag should be > implemented in the same way as -mbranch-likely? -mllsc is a little different in that it can be used even when the ISA doesn't support it (thanks to kernel emulation). -mimadd isn't like that though: we only want to use MADD/MSUB if the ISA has it. So I think it makes sense to leave -mllsc as it is but do -mimadd in the same way as -mbranch-likely. Thanks, Richard
diff --git a/gcc/config/mips/mips.h b/gcc/config/mips/mips.h index 0acce14..62a7701 100644 --- a/gcc/config/mips/mips.h +++ b/gcc/config/mips/mips.h @@ -875,7 +875,9 @@ struct mips_cpu_info { && !TARGET_MIPS16) /* Integer multiply-accumulate instructions should be generated. */ -#define GENERATE_MADD_MSUB (ISA_HAS_MADD_MSUB && !TUNE_74K) +#define GENERATE_MADD_MSUB (ISA_HAS_MADD_MSUB \ + && (target_flags_explicit & MASK_IMADD \ + ? TARGET_IMADD : !TUNE_74K)) /* ISA has floating-point madd and msub instructions 'd = a * b [+-] c'. */ #define ISA_HAS_FP_MADD4_MSUB4 ISA_HAS_FP4 diff --git a/gcc/config/mips/mips.opt b/gcc/config/mips/mips.opt index d8ef2e7..6b3024b 100644 --- a/gcc/config/mips/mips.opt +++ b/gcc/config/mips/mips.opt @@ -58,6 +58,10 @@ mmad Target Report Var(TARGET_MAD) Use PMC-style 'mad' instructions +mimadd +Target Report Mask(IMADD) +Use integer madd/msub instructions + march= Target RejectNegative Joined Var(mips_arch_option) ToLower Enum(mips_arch_opt_value) -march=ISA Generate code for the given ISA