> -----Original Message----- > From: gcc-patches-ow...@gcc.gnu.org [mailto:gcc-patches- > ow...@gcc.gnu.org] On Behalf Of Matthew Fortune > Sent: Monday, January 16, 2017 11:25 AM > To: Doug Gilmore <doug.gilm...@imgtec.com>; gcc- > patc...@gcc.gnu.org > Cc: Moore, Catherine <catherine_mo...@mentor.com> > Subject: RE: [PATCH, MIPS] Target flag and build option to disable > indexed memory OPs. > > Doug Gilmore <doug.gilm...@imgtec.com> > > I recently bisected PR78176 to problems introduced with r21650. > > > > Given the short time until the release, we would like to provide a > > target flag and build option to avoid the bug until we are able to > > resolve the problem with the commit. Note that as Matthew Fortune > has > > mentioned in the PR: > > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78176#c5 > > > > the problem could also be addressed by updates to the Linux kernel > since > > the problem is only exposed by running MIPS 32-bit binaries on 64- > bit > > kernels. > > > > Bootstrapped on X86_64, regression tested on X86_64 and MIPS. > > > > OK to commit? > > Given this is a generic reference to indexed load/store and the issue > could > affect any indexed operation then I think it needs to include all of the > following as well: > > /* ISA has lwxs instruction (load w/scaled index address. */ > #define ISA_HAS_LWXS ((TARGET_SMARTMIPS || > TARGET_MICROMIPS) \ > && !TARGET_MIPS16) > > /* ISA has lbx, lbux, lhx, lhx, lhux, lwx, lwux, or ldx instruction. */ > #define ISA_HAS_LBX (TARGET_OCTEON2) > #define ISA_HAS_LBUX (ISA_HAS_DSP || TARGET_OCTEON2) > #define ISA_HAS_LHX (ISA_HAS_DSP || TARGET_OCTEON2) > #define ISA_HAS_LHUX (TARGET_OCTEON2) > #define ISA_HAS_LWX (ISA_HAS_DSP || TARGET_OCTEON2) > #define ISA_HAS_LWUX (TARGET_OCTEON2 && TARGET_64BIT) > #define ISA_HAS_LDX ((ISA_HAS_DSP || TARGET_OCTEON2) \ > && TARGET_64BIT) > > The DSP LBUX/LHX/LWX/LDX intrinsics will also need a new AVAIL > predicate > to disable them. The snag is that some DSP code will fail to compile if it > uses the DSP load intrinsics directly. > > I see no way of avoiding that. Therefore, distributions that use > --without-indexed-load-store will have to cope with some potential > DSP > fallout if they enable DSP at all. > > @Catherine: I'd like your input here if possible as I advocated this > approach, comments on option names welcome too. I quite like the > verbose > name.
Okay, based on my reading of the comments in the bug report, you are proposing this option as a workaround to a kernel deficiency. I don't see any agreement that this is actually a compiler bug. Do we really need to include the DSP instrinsics as well? Do you think that many distributions actually enable DSP? The option name itself is acceptable to me. I'd like to see documentation that explains when this problem is exposed. I'd like to limit the fix to LWXS and I'd like to see the testcase from the bug report added to the testsuite. I also agree that the preprocessor macro is a good idea (even if we decide to forgo the DSP portion of the patch). Catherine > > @Doug: Have you tried running the testsuite with the configure option > --without-indexed-load-store? There may be tests that need adjusting > where they > test indexed load/store. We probably need a pre-processor macro > to detect if the option is enabled/disabled so that DSP code can be > predicated > on indexed load being available. > > Thanks, > Matthew
